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

On 08/08, Junio C Hamano wrote:

Thomas Gummerer [off-list ref] writes:

quoted

quoted

quoted

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