Am 10.05.2018 um 12:51 schrieb Junio C Hamano: > René Scharfe <l.s.r@xxxxxx> writes: > >> The standard says about uintptr_t that "any valid pointer to void can >> be converted to this type, then converted back to pointer to void, and >> the result will compare equal to the original pointer". So void * -> >> uintptr_t -> void * is a proper roundtrip, but that doesn't imply that >> casting arbitrary uintptr_t values to void * would be lossless. >> >> I don't know an architecture where this would bite us, but I wonder if >> there is a cleaner way. Perhaps changing the type of the decoration >> member of struct decoration_entry in decorate.h to uintptr_t? > > In order to ensure "void * -> uintptr_t -> void *" roundtrip holds, > the implementation would guarantee that uintptr_t is wider than > void*, so what you suggest technically makes sense. We should be > able to store any pointer in the field. And we should be able to > store any value of an unsigned integral type that is narrower than > uintptr_t. > > But it somehow feels backwards in spirit to me, as the reason why we > use "void *" there in the decoration field is because we expect that > we'd have a pointer to some struture most of the time, and we have > to occasionally store a small integer there. Yes, fast-export seems to be the only place that stores an integer as a decoration. > So I'd naively expect > that > > uint32_t mark = 23; > de->decoration = (void *)mark; > > would be a good way to store mark #23 in the field and > > uint32_t mark; > mark = (typeof(mark))de->decoration; > > would be a good way to read it off of the "void *" field. Of > course, this assume that (void *) is at least as wide as 32-bit and > it also ignores the standard ;-) Right, it looks deceptively good and works fine if memory is flat and valid values for pointers are in a contiguous range starting at zero. The standard allows for other models as well, though. > This is an unrelated tangent but the mark-to-ptr() and ptr-to-mark() > implementations feel wasteful, especially when we worry about 32-bit > archs. A naive platform implementation of > > (uint32_t *)mark - (uint32_t *)NULL; > > would be ((uintptr_t)mark) / 4, i.e. the de->decoration field will > always have two LSB clear and only utilize top 30-bit to represent > the value of mark. That's right, but I don't see what's naive about it, or how a 32-bit architecture could avoid wasting those two bits. Using struct decorate in fast-export has the benefit of not requiring separate allocations for individual entries. Switching to struct hashmap would require individual allocations. Adding a custom clone of decorate with a uint32_t payload would be an option. René
