Re: [PATCH/RFC v2 09/16] Read index-v5

[Thomas Gummerer <t.gummerer@gmail.com>]

On 08/08, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> >> > +	name = (char *)mmap + *dir_offset;
> >> > +	beginning = mmap + *dir_table_offset;
> >> 
> >> Notice how you computed name with pointer arithmetic by first
> >> casting mmap (which is "void *") and when computing beginning, you
> >> forgot to cast mmap and attempted pointer arithmetic with "void *".
> >> The latter does not work and breaks compilation.
> >> 
> >> The pointer-arith with "void *" is not limited to this function.
> > ...
> > I've used the type of the respective assignment for now. e.g. i have
> > struct cache_header *hdr, so I'm using
> > hdr = (struct cache_header *)mmap + x;
> 
> You need to be careful when rewriting the above to choose the right
> value for 'x' if you go that route (which I wouldn't recommend).
> 
> With
> 
>     hdr = ptr_add(mmap, x);
> 
> you are making "hdr" point at x BYTES beyond mmap, but
> 
>     hdr = (struct cache_header *)mmap + x;
> 
> means something entirely different, no?  "hdr" points at x entries
> of "struct cache_header" beyond mmap (in other words, if mmap[] were
> defined as "struct cache_header mmap[]", the above is saying the
> same as "hdr = &mmap[x]").
> 
> I think the way you casted to compute the value for the "name"
> pointer is the (second) right thing to do.  The cast (char *)
> applied to "mmap" is about "mmap is a typeless blob of memory I want
> to count bytes in.  Give me *dir_offset bytes into that blob".  It
> is not tied to the type of LHS (i.e. "name") at all.  The result
> then needs to be casted to the type of LHS (i.e. "name"), and in
> this case the types happen to be the same, so you do not have to
> cast the result of the addition but that is mere luck.
> 
> The next line is not so lucky and you would need to say something
> like:
> 
>     beginning = (uint32_t *)((char *)mmap + *dir_table_offset);
> 
> Again, inner cast is about "mmap is a blob counted in bytes", the
> outer cast is about type mismatch between a byte-address and LHS of
> the assignment.

This is what I tried in v3 of the series, but it didn't seem quiet
right.

> If mmap variable in this function were not "void *" but something
> more sane like "const char *", you wouldn't have to have the inner
> cast to begin with, and that is why I said the way you did "name" is
> the second right thing.  Then you can write them like
> 
>     name = mmap + *dir_offset;
>     beginning = (uint32_t *)(mmap + *dir_offset);
> 
> After thinking about this, the ptr_add() macro might be the best
> solution, even though I originally called it as a band-aid.  We know
> mmap is a blob of memory, byte-offset of each component of which we
> know about, so we can say
> 
>     name = ptr_add(mmap, *dir_offset);
>     beginning = ptr_add(mmap, *dir_offset);
> 
> Hmmm..

I start to think so too. Casting the mmap variable to "const char *"
in the method call doesn't feel right to me, even though it would work.
Unless there are any objections I'll use ptr_add in the next version.