Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute

From: Jeff King <peff@peff.net>
Date: Jan 23, 2018, 00:54 UTC
Message-ID: <20180123005401.GG26357@sigill.intra.peff.net>
In-Reply-To: <05265803-BD74-4667-ABB5-9752E55A5015@gmail.com>

On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote:

Show 20 quoted lines

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