On Fri, Jun 5, 2015 at 4:45 PM, Jeff King <peff@xxxxxxxx> wrote: > On Fri, Jun 05, 2015 at 01:41:21AM +0000, brian m. carlson wrote: > >> However, with the object_id conversion, we run into a problem: casting >> those unsigned char * values into struct object_id * values is not >> allowed by the C standard. There are two possible solutions: copying; >> and the just-do-it solution, where we cast and hope for the best. > > I'm not sure if it does violate the standard. The address of the first > element of a struct is guaranteed to match the address of the struct > itself. The object_id.hash member is an array of unsigned char, so there > are no alignment issues. It might run afoul of rules about casting > between pointer types (i.e., pointers for all types are not guaranteed > to be the same size). The standard dictates that "char *" and "void *" > are the same size, and big enough to hold a pointer to anything, so I > think it might even be OK. > > But I'm not even sure that line of thinking is all that interesting. > Even if we are violating some dark corner of the standard, this > definitely falls into the "it's useful and works on all sane machines" > category. We also do much worse things with struct-casting mmap'd data > elsewhere (e.g., see the use of "struct pack_header"). It works fine in > practice as long as you are careful about alignment and padding issues. > > So my vote would be to retain the cast. This is very low-level, > performance-sensitive code. I did some very naive timings and didn't see > any measurable change from your patch, but I also don't think we are > seeing a real portability benefit to moving to the copy, so I'd prefer > to keep the status quo. I'm more concerned about breaking object_id abstraction than C standard. Let's think a bit about future. I suppose we need to support both sha-1 and sha-512, at least at the source code level. That might make casting tricky. Maybe we should deal with it now instead of delaying because if the final solution is vastly different, we may be redoing this conversion again. In any case, if we cast, we should make it grep-able (maybe hide the casting in a macro so we can grep the macro's name) so we can examine them when the time comes for us to move away from sha-1. -- Duy -- 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