On Dec 19 09:59, Ken Brown wrote:
> Hi Corinna,
>
> On 12/19/2024 5:17 AM, Corinna Vinschen wrote:
> > Fixes: c68de3a262fe5 ("* mmap.cc (class mmap_record): Declare new map_pages
> > method with address parameter.")
>
> Again thanks. Some time you'll have to tell me how to find those commits.
I'm using `git blame <filename>' and check relevant commit IDs backwards
in time until it fits. When I find a commit 123456789 which changes the
lines but is not actually the one introducing the problem, next I use
git blame <filename> 123456789^ <--Note the caret
For files changing their name, you have to change the filename on
the commandline, too. For instance
git blame mm/mmap.cc
but
git blame mmap.cc 123456789^
if the commit is before the filename change.
> > > + if (u_addr > (caddr_t) addr || u_len < len
> > > || !rec->compatible_flags (flags))
> >
> > While this is strictly correct, I wonder if this shouldn't be
> >
> > if (u_addr > (caddr_t) addr || u_addr + u_len < (caddr_t) addr + len ...
> >
> > for plain readability. The problem is, you can't see what match()
> > really returns, an intersection or the entire free region. That's
> > what I stumbled over in the cygwin ML.
> >
> > This way, the code immediately tells the reader that we want to make
> > sure that [addr,addr+len] is a region completely inside the region
> > [u_addr,u_addr+u_len], without needing to know what exactly match()
> > returns. And it would still be correct, even if we redefine match().
> >
> > What do you think?
> I agree. I'll make that change and push. At some point it might be helpful
> to add a reference parameter "contains" to match(), so that match() can
> return the information about whether the mmap_record region contains
> [addr,addr+len]. That way the relevant tests can be done right where the
> reader can see what's going on. But I'm not going to try to do that
> immediately.
Sounds like a good extension. It would be great if we could handle
partial page intersections at one point, too.
Corinna
