Re: [PATCH v3 2/8] diff: convert flags to be stored in bitfields

From: Stefan Beller <sbeller@google.com>
Date: Oct 31, 2017, 21:32 UTC
Message-ID: <CAGZ79kajX=FmGacDSv_dDzppE6WBHGqgyCpaG1zDyENcsDbwZg@mail.gmail.com>
In-Reply-To: <20171031181911.42687-3-bmwill@google.com>

On Tue, Oct 31, 2017 at 11:19 AM, Brandon Williams <bmwill@google.com> wrote:

Show 7 quoted lines

> We cannot add many more flags to the diff machinery due to the
> limitations of the number of flags that can be stored in a single
> unsigned int.  In order to allow for more flags to be added to the diff
> machinery in the future this patch converts the flags to be stored in
> bitfields in 'struct diff_flags'.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for this cleanup series! Stefan

> +struct diff_flags {
> +       unsigned RECURSIVE:1;

After some quick research our coding style on bit fields is twofold: Most older code is this way and more recent code seems to prefer

    unsigned <FLAGNAME> SP : SP ;

If this turns out to be the only nit, I would ignore it for the sake of faster settlement of the series.

Show 8 quoted lines

> +static inline void diff_flags_or(struct diff_flags *a,
> +                                const struct diff_flags *b)
> +{
> +       char *tmp_a = (char *)a;
> +       const char *tmp_b = (const char *)b;
> +       int i;
> +
> +       for (i = 0; i < sizeof(struct diff_flags); i++)

I think most of the code prefers to put the variable into the sizeof argument i.e. 'sizeof(*a)', as that is presumably more maintainable. (If the type of 'a' changes, then we don't forget to adapt this place, but the compiler can take care of it.