On 26/01/27 07:29AM, Patrick Steinhardt wrote:

Unfortunately, the other callsite wouldn't see a warning because we pass
an integer constant, and the compiler doesn't complain about that at
all. It also falls apart once you start to OR multiple flags together.

I guess we could have enum values to each of the combinations that get
used together, but that problably isn't a great idea if we use many
different combinations and may not be good in the long term.

It would be great if there was a way to tell the compiler that a given
flags field expects only enum values so that it could always warn about
misuse. But I'm not aware of any way to do this.

I agree with the sentiment. Passing a combination of enum values as
unsigned flags is a bit fragile and in some ways feels like it defeats
the point of using enums to begin with. Also when using a function that
accepts flags, it is not always immediately obvious which set of flags
are expected.

We could of course start to take a more heavy-handed approach and always
accept an options struct instead. E.g.

    struct odb_read_object_info_options {
            unsigned lookup_replace : 1,
                     quick : 1,
                     skip_fetch_object : 1,
                     for_prefetch : 1,
                     die_if_corrupt : 1;
    };

That would give us full type safety, and it would be impossible to
misuse without getting a compiler warning. Furthermore, with designated
initializers it wouldn't be _that_ awful to use:

	if (!odb_read_object_extended(ctx->repo->objects, &list->oid[i],
				      (struct odb_read_object_info_options) {
		.skip_fetch_object = 1,
	}) < 0) {
		die("...");
	}

But I wouldn't exactly call it ergonomic, either.

This would certainly be the most safe option, but I also agree that it
not particually ergonomic. I'm not sure it's worth going this far as
long as it's documented/easy-to-find the corresponding set of flags.

So I'm not sure whether this partial protection would be worth it, but
if you think it is I'm happy to reroll.

I think this version of series is good and doesn't need a reroll.

Thanks,
-Justin