Re: [PATCH 4/5] config.c: add a "tristate" helper

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 9, 2021, 01:33 UTC
Message-ID: <87o8eogs2r.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqtuogpc6f.fsf@gitster.g>

On Fri, Apr 09 2021, Junio C Hamano wrote:

Show 10 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I'd prefer to just make these "enum", which means we'll have the aid of
>> the compiler in checking all the callsites, as in the patch-on-top
>> (which I can squash appropriately, need to update the doc comments
>> though) at the end of this E-Mail.
>
> I think enum is oversold by some people (not me).  C Compilers won't
> do much when you use them interchangeably with int, simply because
> they are designed to be used that way, no?

They are just ints, not a seperate type like in C++.

Show 7 quoted lines

> If existing code used 0 as false and 1 as true, and it learns an
> "auto" value with a new definition,
>
>     #define TRISTATE_AUTO 2
>
> without TRISTATE_{TRUE,FALSE} defined to 0 and 1, that would be a
> good place to stop.  I'd be quite happy with that.

The benefit in this case is to human readers of the code who know they're being helped by the enum checking in "case" arms.

So you can immediately look at code like:

    enum foo x = git_config_foo();
    switch (x)
    [...]

And know without having to jump to the definition of "git_config_foo()" that all the return values are being handled.

Just to clarify, do you have a dislike of the sort of code in http://lore.kernel.org/git/875z0wicmp.fsf@evledraar.gmail.com for all uses in the codebase, or in this case because the diff of adapting existing code is larger as a result?

I think if we're not going to use enum/switch for this and similar things in config.c in the future it makes sense to pass a pointer to a "is_auto" parameter to these new tristate() functions, similar to e.g. the existing git_config_bool_or_int().