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

On Tue, May 11, 2021 at 4:14 AM Junio C Hamano [off-list ref] wrote:

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

+/*
+ * 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.

quoted

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