Re: [PATCH v2 2/2] [GSOC] ref-filter: introduce enum atom_type

From: Christian Couder <christian.couder@gmail.com>
Date: May 11, 2021, 05:51 UTC
Message-ID: <CAP8UFD3m0TA3nPDaPhaU3=FVF_r5cPM8xksVL6MpCOhNC4dT3g@mail.gmail.com>
In-Reply-To: <xmqqh7jaow31.fsf@gitster.g>

On Tue, May 11, 2021 at 4:14 AM Junio C Hamano <gitster@pobox.com> wrote:

>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> > +/*
> > + * The enum atom_type is used as the coordinates of valid_atom entry.
> > + * In the atom parsing stage, it will be passed to used_atom.atom_type
> > + * as the identifier of the atom type. We can judge the type of used_atom
> > + * entry by `if (used_atom[i].atom_type == ATOM_*)`.
> > + *
> > + * ATOM_UNKNOWN equals to 0, used as an enumeration value of uninitialized
> > + * atom_type.
>
> Shouldn't it be (-1)?

If it's -1 instead of 0, then it might be a bit more complex to initialize structs that contain such a field, as it cannot be done with only xcalloc().

Show 12 quoted lines

> And I'd assume I am right in the following.
>
> > + * ATOM_INVALID equals to the size of valid_atom array, which could help us
> > + * iterate over valid_atom array like this:
> > + *
> > + *   for (i = ATOM_UNKNOWN + 1; i < ATOM_INVALID; i++) {
>
> I find it far more intuitive to say
>
>         for (i = 0; i < ATOM_INVALID; i++)
>
> than having to say UNKNOWN+1.

Yeah, that's more intuitive. But in my opinion, using `ATOM_UNKNOWN + 1` instead of `0` at least shouldn't often result in more lines of code, and should be a bit easier to get right, compared to having to initialize the field with ATOM_UNKNOWN.

Show 10 quoted lines

> In any case, the values should be indented, and a comment should
> ensure that the final one stays at the end, perhaps like this.
>
>         enum atom_type {
>                 ATOM_INVALID = -2,
>                 ATOM_UNKNOWN = -1,
>                 ATOM_REFNAME,
>                 ...
>                 ATOM_ELSE,
>                 ATOM_MAX /* MUST BE AT THE END */

I agree that a comment telling people that it must be at the end is good.

>         }
>
> (note that the trailing comma is deliberately omitted).

Yeah.

Show 5 quoted lines

> It would allow people to say
>
>         for (i = 0; i < ATOM_MAX; i++)
>
> instead, which would be even nicer.

Yeah, it's also a tradeoff to have the last one called ATOM_MAX instead of ATOM_INVALID, and to have a separate ATOM_INVALID if it's needed.