From: Jeff King <peff@peff.net>
Date: Thu, 22 Jan 2026 19:23:37 GMT
Subject: Re: [PATCH v3 02/14] odb: fix flags parameter to be unsigned
Message-ID: <20260122192337.GC2098026@coredump.intra.peff.net>
In-Reply-To: <xmqqcy31pscg.fsf@gitster.g>

On Thu, Jan 22, 2026 at 07:41:51AM -0800, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> > I agree with you that we should be using an enum in these cases over
> > unsigned for the reasons you suggest. I've stumbled over this in the
> > past, so perhaps this is worth adding to the CodingGuidelines?
> 
> I am OK with declaring our preference of "enum" over "#define"d
> constants.  The only two minor hesitation I have against the use of
> "enum", especially for bitset but not for enumeration, are that

I don't think there's any disagreement over using enums in general. It's
just a question of what type to declare in function interfaces.

>  (1) enum gives a false sense of type safety to casual coders. If I
>      have two enum types and pass one to as a parameter to a
>      function that expects the other one, would the compiler help me
>      catch that as a potential mistake?  -Wenum-conversion is not
>      enabled even with -Wall so I am assuming that the compiler
>      folks fells that it is not reliable enough.

It is enabled with -Wextra, which we turn on with DEVELOPER=1. I think
gcc will catch the most obvious mismatches like:

  enum one { FOO };
  enum two { BAR };
  void func(enum one value);
  void doit(void) { func(BAR); }

which yields:

  $ gcc -c -Wall -Wextra foo.c
  foo.c: In function ‘doit’:
  foo.c:4:24: warning: implicit conversion from ‘enum two’ to ‘enum one’ [-Wenum-conversion]
      4 | void doit(void) { func(BAR); }
        |                        ^~~

What it doesn't help with is passing arbitrary integers, which includes
#define'd constants. Swapping out "enum two" for:

  #define BAR 1

will not produce a warning. That's the issue that I ran into with the
color code in:

  https://lore.kernel.org/git/20250916202748.GM612873@coredump.intra.peff.net/

Unfortunately bit operations on enum values seem to lose the "type" for
the purposes of this warning, and just become regular integers. So if we
modify our example to:

  num one { FOO_A = 1 << 0, FOO_B = 1 << 1 };
  enum two { BAR_A = 1 << 0, BAR_B = 1 << 1 };
  void func(enum one value);
  void doit(void) { func(BAR_A | BAR_B); }

it no longer complains.

I still think we are better off declaring the flag parameters with the
enum type, though. It will catch some problematic cases. And even if
there were no compiler support at all, I think the hint to humans about
the expected type is worth it.

>  (2) it is not easy to force an enum type to be unsigned, unless you
>      are at C23 or above.  If shifting enums are warned by the
>      compilers by default, I wouldn't worry about it, but use of
>      unsigned is more explicit in this regard.

Do we need to force unsignedness for bit-flags? The compiler will use a
type that is sufficiently large for the enum values defined, and I would
not expect anybody to shift them. Only to construct them with bitwise-OR
and check them with bitwise-AND.

-Peff