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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2012, 16:49 UTC
Message-ID: <7vr4rhtjkd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120808074138.GD867@tgummerer.surfnet.iacbox>

Thomas Gummerer <t.gummerer@gmail.com> writes:

Show 13 quoted lines

>> > +	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.

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..