"ZheNing Hu via GitGitGadget" [off-list ref] writes:
the enum value of `ATOM_UNKNOWN` is equals to zero, which
s/the/The/;
could ensure that we can easily distinguish such a struct
where the atom_type is known from such a struct where it
is unknown yet.
the enum value of `ATOM_INVALID` is equals to the size of
Ditto.
+/*
+ * 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)?
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.
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 */
}
(note that the trailing comma is deliberately omitted).
It would allow people to say
for (i = 0; i < ATOM_MAX; i++)
instead, which would be even nicer.
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.
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.
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 */
}
(note that the trailing comma is deliberately omitted).
It would allow people to say
for (i = 0; i < ATOM_MAX; i++)
instead, which would be even nicer.
I think ATOM_INVALID and ATOM_MAX all will have a
similar effort. Why don't we omit one of them?
For the time being, all the used_atom entry create in
`parse_ref_filter_atom()`, we directly use
`used_atom[at].atom_type = i;` after realloc the used_atom.
There is no time for "ATOM_UNKNOWN" at all.
I don’t know if it makes a lot of sense use "ATOM_UNKNOWN"
at the moment.
--
ZheNing Hu
Christian Couder [off-list ref] 于2021年5月11日周二 下午1:51写道：
quoted
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().
I agree. If the traverse start from 0, an init atom_type will have
"ATOM_REFNAME" junk value. If let users manually adjust it to
ATOM_UNKNOWN, it seems to be very troublesome.
quoted
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.
There will be a trade-off. Traverse from 0 or does not need to adjust the
initialized atom_type = UNKNOWN.
quoted
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.
About ATOM_MAX or ATOM_INVALID, I have a idea:
enum atom_type {
ATOM_UNKNOWN,
...
ATOM_ELSE,
ATOM_INVALID,
+ATOM_MAX = ATOM_INVALID
};
This might be able to do both.
Thanks.
--
ZheNing Hu