Re: [PATCH v2] refs.h: make all flags arguments unsigned

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 3, 2022, 17:53 UTC
Message-ID: <220203.8635kz6d2o.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAFQ2z_Oxq67XO20oG7Tokk48wm5ZzHRR3GDi4PG5wg7FMJ89aA@mail.gmail.com>

On Thu, Feb 03 2022, Han-Wen Nienhuys wrote:

Show 25 quoted lines

> On Wed, Feb 2, 2022 at 12:03 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@evledraar.gmail.com/ 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.