On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote: > >> + enc = xcalloc(1, sizeof(struct convert_driver)); > > > > I think this should be "sizeof(struct encoding)" but I prefer > > "sizeof(*enc)" which prevents these kind of mistakes. > > Great catch! Thank you! > > Other code paths are at risk of this problem too. Consider this: > > $ git grep 'sizeof(\*' | wc -l > 303 > $ git grep 'sizeof(struct ' | wc -l > 208 > > E.g. even in the same file (likely where I got the code from): > https://github.com/git/git/blob/59c276cf4da0705064c32c9dba54baefa282ea55/convert.c#L780 > > @Junio: > Would you welcome a patch that replaces "struct foo" with "*foo" > if applicable?
This is part of the reason we've been moving to helpers like ALLOC_ARRAY(), which make it harder to get this wrong. We don't have an ALLOC_OBJECT(), which is what you would want here. I'm not sure if that is helpful or crossing the line of "you're obscuring it to the point that people familiar with C have trouble reading the code". The ALLOC_ARRAY() macros have been sort of an experiment there (I tend to like them, but I also work with Git's code often enough that I am not likely to be confused by our bespoke macros). But anyway, that was a bit of a tangent. Certainly the smaller change is just standardizing on sizeof(*foo), which I think most people agree on at this point. It might be worth putting in CodingGuidelines. -Peff
