On Tue, Mar 17, 2020 at 04:53:44PM -0400, Eric Sunshine wrote:
> > + case LOFC_DISABLED:
> > + case LOFC__COUNT:
> > + /*
> > + * Include these to catch all enumerated values, but
> > + * break to treat them as a bug. Any new values of this
> > + * enum will cause a compiler error, as desired.
> > + */
>
> In general, people will see a warning, not an error, unless they
> specifically use -Werror (or such) to turn the warning into an error,
> so this statement is misleading. Also, while some compilers may
> complain, others may not. So, although the comment claims that we will
> notice an unhandled enum constant at compile-time, that isn't
> necessarily the case.
Yes, but that's the best we can do, isn't it?
There's sort of a meta-issue here which Taylor and I discussed off-list
and which led to this comment. We quite often write switch statements
over enums like this:
switch (foo)
case FOO_ONE:
...do something...
case FOO_TWO:
...something else...
default:
BUG("I don't know what to do with %d", foo);
}
That's reasonable and does the right thing at runtime if we ever hit
this case. But it has the unfortunate side effect that we lose any
-Wswitch warning that could tell us at compile time that we're missing a
case. Not everybody would see such a warning, as you note, but
developers on gcc and clang generally would (it's part of -Wall).
But we can't just remove the default case. Even though enums don't
generally take on other values, it's legal for them to do so. So we do
want to make sure we BUG() in that instance.
This is awkward to solve in the general case[1]. But because we're
returning in each case arm here, it's easy to just put the BUG() after
the switch. Anything that didn't return is unhandled, and we get the
best of both: -Wswitch warnings when we need to add a new filter type,
and a BUG() in the off chance that we see an unexpected value.
But the cost is that we have to enumerate the set of values that are
defined but not handled here (LOFC__COUNT, for instance, isn't a real
enum value but rather a placeholder to let other code know how many
filter types there are).
So...I dunno. Worth it as a general technique?
-Peff
[1] In the general case where you don't return, you have to somehow know
whether the value was actually handled or not (and BUG() if it
wasn't). Presumably by keeping a separate flag variable, which is
pretty ugly. -Wswitch-enum is supposed to deal with this by
requiring that you list all of the values even if you have a default
case. But it triggers in a lot of other places in the code that I
think would be made much harder to read by having to list out the
enumerated possibilities.
