Re: [PATCH v4 1/7] Add delta-islands.{c,h}

From: Jeff King <peff@peff.net>
Date: Aug 13, 2018, 18:11 UTC
Message-ID: <20180813181149.GA10013@sigill.intra.peff.net>
In-Reply-To: <7a780fe9-e8bf-804a-82e6-8df81cd5c41c@ramsayjones.plus.com>

On Mon, Aug 13, 2018 at 01:17:18PM +0100, Ramsay Jones wrote:

Show 29 quoted lines

> >>> +struct island_bitmap {
> >>> +     uint32_t refcount;
> >>> +     uint32_t bits[];
> >>
> >> Use FLEX_ARRAY here? We are slowly moving toward requiring
> >> certain C99 features, but I can't remember a flex array
> >> weather-balloon patch.
> > 
> > This was already discussed by Junio and Peff there:
> > 
> > https://public-inbox.org/git/20180727130229.GB18599@sigill.intra.peff.net/
> > 
> 
> That is a fine discussion, without a firm conclusion, but I don't
> think you can simply do nothing here:
> 
>   $ cat -n junk.c
>        1	#include <stdint.h>
>        2	
>        3	struct island_bitmap {
>        4		uint32_t refcount;
>        5		uint32_t bits[];
>        6	};
>        7	
>   $ gcc --std=c89 --pedantic -c junk.c
>   junk.c:5:11: warning: ISO C90 does not support flexible array members [-Wpedantic]
>     uint32_t bits[];
>              ^~~~
>   $ gcc --std=c99 --pedantic -c junk.c

Right, whether we use the FLEX_ALLOC macros or not, this needs to be declared with FLEX_ARRAY, not an empty "[]".

I'm fine either way on using the FLEX_ALLOC macros.

Show 8 quoted lines

> >> ... Ah, OK, trg_ => target.
> > 
> > I am ok to replace "trg" with "target" (or maybe "dst"? or something
> > else) and "src" with "source" if you think it would make things
> > clearer.
> 
> If it had been dst_ (or target), I would not have had a 'huh?'
> moment, but it is not all that important.

FWIW, these are all inherited from try_delta(), etc, in the existing code. I'm fine with using another term, but we should probably do it universally. And if we do, probably "base" is a better name than "src", since the direction depends on which part of the relationship you are considering. I'm not sure what that makes "dst".

-Peff