From: Patrick Steinhardt <ps@pks.im>
Date: Fri, 23 Jan 2026 10:57:56 GMT
Subject: Re: [PATCH v3 02/14] odb: fix flags parameter to be unsigned
Message-ID: <aXNUNJudud_KuT33@pks.im>
In-Reply-To: <20260122192337.GC2098026@coredump.intra.peff.net>

On Thu, Jan 22, 2026 at 02:23:37PM -0500, Jeff King wrote:
> 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.

I don't care strongly enough myself, but do you or Taylor maybe want to
send a patch that documents our preference? If so I'll be happy to adapt
my series to use whatever style we agree on.

Thanks!

Patrick