On 08/08, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@xxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html