On Thu, Feb 03 2022, Han-Wen Nienhuys wrote:
> On Wed, Feb 2, 2022 at 12:03 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>> > The post-image LGTM, but I'm also a bit "meh" on the churn just for
>> > signed->unsigned, especially given the conflict with my in-flight
>> > ab/no-errno-from-resolve-ref-unsafe. But it's not too bad, and if Junio
>> > hasn't complained about it...
>>
>> I won't complain myself. I'd still try to help newer developers,
>> but my intention is to make it the responsibility for individual
>> developers to make sure their topic works well with topics in
>> flight ;-)
>
> I'm sending v3 based on seen.
>
>> Between "enum" and #define that is stored in "unsigned", neither
>> gives us much type safety in C; "enum" may be somewhat worse by
>> giving a false sense of having a type safety that does not really
>> exist, than "unsigned int" that is more honestly defeats such a
>> false sense of safety. So I have no strong preference either way.
>
> Neither gives true type safety, and I don't know if an enum is kosher
> at all; shouldn't the value always be one of the enumerees, strictly
> speaking?
No, it's nice so you can use it in switch/case, but it's also a
perfectly legit use-case to use it for bitfields.
And as I noted e.g. gdb will understand that and give you pretty-printed
flags based on that, which is very nice for debugging.
And it's also just nice for readability and source navigation. I.e. if
it's "unsigned int foo_flags" and I find "foo_flags" with [ce]tags I'll
only find all other uses of "foo_flags".
Whereas the enum will give me its definition, which usually has comments
etc.
To be fair it's usually easy to find it even without that, because
you'll find a use of a relevant "#define" pretty soon, and can navigate
to that. But sometimes that's 1-2 functions away if the flag is being
passed along.
So being able to navigate straight from the function arguments is nice,
as you can do with structs and other types.
> I proposed both options because a distinct typename lets me jump to
> the definition of the flags easily through ctags.
I'm not sure I understand you here. I use ctags (via Emacs) and it's
perfectly capable of finding both "enum xyz" and "typedef enum { ... }
xyz":
$ make TAGS
$ grep -e rewrite_result -e parse_opt_option_flags TAGS
static enum rewrite_result line_log_rewrite_one(1285,32804
enum parse_opt_option_flags 39,772
enum parse_opt_option_flags flags;137,4133
static enum rewrite_result rewrite_one_1(3608,101152
static enum rewrite_result rewrite_one(3645,102081
enum rewrite_result 445,12193
typedef enum rewrite_result (*rewrite_parent_fn_t)rewrite_parent_fn_t451,12279
Hrm, that's etags actually, but the same is true of "make tags":
$ grep -e rewrite_result -e parse_opt_option_flags tags
line_log_rewrite_one line-log.c /^static enum rewrite_result line_log_rewrite_one(st/
parse_opt_option_flags parse-options.h 39
rewrite_one_1 revision.c /^static enum rewrite_result rewrite_one_1(struct re/
rewrite_one revision.c /^static enum rewrite_result rewrite_one(struct rev_/
rewrite_result revision.h 445
In any case, both [ce]tags find a typdef'd and non-typedef'd variant,
don't they?
> Another idea is to mark the type of the flags by its name, eg.
> transaction_flags, resolve_flags, reftype_flags etc. This wouldn't
> help with ctags, but it does help with readability.
Yes, enums or not, what I was also pointing out in
https://lore.kernel.org/git/220201.86ilty9vq2.gmgdl@xxxxxxxxxxxxxxxxxxx/
is that changing just one logical set of flags at a time would make this
much easier to review.
It doesn't matter for the end result as long as we end up with "unsigned
int" everywhere, but would with enums.
