From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 22 Jan 2026 06:50:58 GMT
Subject: Re: [PATCH v3 02/14] odb: fix flags parameter to be unsigned
Message-ID: <aXHI0vNArKiDCL-I@pks.im>
In-Reply-To: <20260121211128.GB723458@coredump.intra.peff.net>

On Wed, Jan 21, 2026 at 04:11:28PM -0500, Jeff King wrote:
> On Wed, Jan 21, 2026 at 01:50:18PM +0100, Patrick Steinhardt wrote:
> 
> > The `flags` parameter accepted by various `for_each_object()` functions
> > is a bitfield of multiple flags. Such parameters are typically unsigned
> > in the Git codebase, but we use `enum odb_for_each_object_flags` in
> > some places.
> 
> I agree that using "unsigned" instead of "int" for flags is a good
> practice in general. But isn't using "unsigned" instead of an enum
> strictly worse?
> 
> The enum is more descriptive to human readers (since the type defines
> which flags we expect to see). And it lets the compiler use the correct
> type in the few cases where it might matter. E.g., if you imagine an
> enum that defines 40 bits, then the compiler will know that it needs to
> use a type larger than 32 bits to store it. Whereas passing a raw
> "unsigned" will truncate some values.
> 
> I don't expect this latter reason to be common, but if we are going to
> have a general principle for how to pass flags, it feels like passing
> the enum (assuming the flags are defined in one) is always better. And
> IMHO just the first reason (human readers) makes it worth doing that way
> anyway.

I'd agree if we used the enum as a plain value directly. But in case
we're using it as a bitset I think it muddies the waters a bit, and I
had the understanding that we typically want to use `unsigned` for
flag bitsets like this.

I think the reason I'm a bit torn is that I'm not a huge fan of having
enum values that don't fall into the range of valid enums. It's valid C
of course, but it just smells weird to me.

> You can find this pattern in lots of places (try grepping for "enum
> [a-z_]* flag"). The ones that aren't are typically using flags that are
> not using enums at all (just #defines).

True, but `unsigned flags` is way more common:

    $ git grep 'unsigned flags' | wc -l
    219

    $ git grep 'enum [a-z_]* flag' | wc -l
    56

In any case, I don't feel too strongly about all of this. I'm happy to
adapt if there is general consensus that we want to use enums instead,
but if so I'd like us to document this in our coding guidelines.

Thanks!

Patrick