On Tue, May 11, 2021 at 4:14 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> > +/*
> > + * 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().
> 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.
> 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.
> 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.
