Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/

From: Pip Cet <[email-masked]>
Date: Fri, 31 Jan 2025 12:48:02 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Fri, 31 Jan 2025 12:15:04 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> > What should one do if the enumeration is large and the code wants to
>> > treat all but the few values the same?  This is a legitimate use case,
>>
>> We have several options:
>>
>> 0. do nothing, keeping bidi.o on the list of unfixed Wswitch objects,
>> explaining that too many large switches are in use for the result of (1)
>> to be readable
>> 1. bite the bullet and apply a 184-line diff to bidi.c.
>> 2. add #pragma statements in bidi.c around the five large switch
>> statements, giving us the benefit of warnings for the other switch
>> statements in bidi.c
>> 3. turn bidi_type_t into a real integer type to avoid the warning
>> 4. cast bidi_type_t to a real integer type in the switch statement.
>> -1. use if() instead
>> -2. use arrays instead
>> -3. use arrays of function pointers instead
>
> That might be fine when the enumeration is ours, but if it comes from
> a system header file, the only practical option is 0, with 1 being a
> very distant second (having a switch with many dozens of values is
> hardly a good thing).

You're talking about a different problem then.  If an
externally-controlled enum is expected to grow, we want a compiler
warning to let us know when this happens, but we also need a default:
handler that deals with this case.  So the right option is that we don't
need any of the extra options, the default behaviour will be just fine.

>> > isn't it?  Having to spell out all of the values is quite tedious, and
>>
>> I don't think the (slight) pain of having to add the case labels is the
>> problem.  I'm willing to do that.
>
> We'll then need to update that each time the library headers add more
> values, so this is not something anyone of us can volunteer to do,
> forever.

It's a compiler warning caused by external code.  Those get added all
the time, and someone usually gets around to fixing or silencing them
eventually.

The last time this happened, our code just silently did the wrong thing
for the new value in an expanded enum.  Having to ignore a warning
letting you know that will happen seems better, IMHO.

>> > eassume will not do what we want in this case.
>>
>> Why not?
>
> Because I'm describing the case where we actually _want_ the default
> handling of an enumerated value to be something valid, whereas eassume
> is for when it's invalid.

Note that in several cases where I've done the conversion, it turns out
that the comment after default: indicated that cases X and Y could be
handled that way, but there's also a case Z that got caught by the
default label and shouldn't have been.  If nothing else, this hints
there are documentation bugs we can catch that way.

>> While "default: eassume (0);" is a weird way of putting it,
>> the semantics with -Werror=switch-enum are "I've handled all enumerated
>> cases explicitly; if the enumeration holds an anonymous value different
>> from all named values, abort in debug builds and don't generate code for
>> this in optimized builds".
>>
>> That applies the same no matter how large the enum is.  What am I
>> missing here?
>
> AFAIU, you are talking about the case where all the enum values were
> spelled out in the switch, in which case eassume is indeed TRT.  But I
> was talking about a different case.

I understand now.  It'd be nice if we could disable -Wswitch-enum for a
specific switch or a specific enum, without using #pragmas, but I don't
think GCC has an option for that yet.  You can use a cast to turn off
the warning with standard GCC.

I think it would be good to tag the bidi_type_t enum so it doesn't
produce such warnings, for example, but only the two switches deep in
bidi_resolve_brackets and bidi_resolve_neutral really seem problematic
at a first glance, to me, and is that worth hacking GCC for?

I'd be perfectly happy making bidi.o a permanent exception (0).  Add a
comment explaining that decision, keep it in the obj list, done.

(I picked bidi.c as an example because it seems to me to be the most
difficult case: the bidi_type_t is large add somewhat tricky, and while
it's defined in the Emacs source code it's ultimately controlled by
Unicode.  If you have a better example, feel free to discuss that instead).

Pip