GNU bug report logs - #75964
Switching the Emacs build to -Wswitch-enum in src/

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75964 <at> debbugs.gnu.org
Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Fri, 31 Jan 2025 12:15:04 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Fri, 31 Jan 2025 09:39:45 +0000
>> From:  Pip Cet via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>> I'm proposing to enable -Wswitch-enum as a warning option when compiling
>> Emacs C sources in src/, and to modify those source files to make good
>> use of it.
>>
>> GCC's -Wswitch-enum will warn about a switch statement of the form
>>
>> enum ABC { A, B, C };
>>
>> enum ABC x = ...;
>>
>> switch (x)
>> {
>>   case A:
>>   case B:
>>     return 1;
>>   default:
>>     return 0;
>> }
>>
>> The reason is that the "default" branch covers both case C and the case
>> that the value of x isn't A, B, or, C.  C allows this latter case and
>> requires compilers to support it, and some Emacs code relies on
>> non-enumerated values to be valid.
>>
>> Instead, with -Wswitch-enum, one should write:
>>
>> enum ABC { A, B, C };
>>
>> enum ABC x = ...;
>>
>> switch (x)
>> {
>>   case A:
>>   case B:
>>     return 1;
>>   case C:
>>     return 0;
>>   default: eassume (false);
>> }
>>
>> assuming x is known never to have a non-enumerated value (this is almost
>> always the case in Emacs).
>
> 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

The negative options are bad and I'd very much advise against them.  (0)
is easiest, and allows us to deal with that problem if we want to change
our approach.  I've done (1), but then I'm having a hard time reading
bidi.c with or without that patch.

(2) is bad because if we ever want to make sure the pragmas disabling
warnings are still appropriate, we've got to find them all.

(3) and (4) avoids the problem with standard GCC, but if my patch is
ever accepted, GCC will start warning about this in some fashion

> 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.  The real harm is to readability, and
that's really important.

> eassume will not do what we want in this case.

Why not?  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?

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.