GNU bug report logs -
#75964
Switching the Emacs build to -Wswitch-enum in src/
Previous Next
Full log
Message #215 received at 75964 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
On 2025-02-05 05:05, Eli Zaretskii wrote:
> There's no way to know, just by looking at the code, whether the
> omission is a mistake or deliberate and required.
Which is exactly why these sorts of patches, if done right, are a good
idea. They make it clear to the compiler and the reader whether these
omission are deliberate.
> I very much doubt these techniques will make our situation
> significantly better, if at all.
Admittedly Emacs has more serious bugs than enum mismatches. However,
checking these mismatches can help find mistakes or help clarify code.
> So basically the plus sign converts the enum to an int, and thus shuts
> up the compiler warnings? If so, why not use an explicit cast
A cast like ((int) E) is too powerful in C. For example, it works even
if E is a pointer, or is floating point. That would introduce more
possibilities for mistakes than +E does. When possible (as is the case
here) we should avoid using casts.
>> For documentation chapter and verse, please see C23 §6.5.4.3 ¶2, which
>> says that the type of +E is like that of -E: the type is E's type after
>> integer promotions.
>
> And which doesn't mention "enum" even once.
It says that the integer promotions are performed. The Emacs enums
promote to int, as shown below.
>> Also please see C23 §6.3.1.1 ¶1–2, which says that
>> int is the type of these Emacs enums after integer promotion.
>
> Which also don't mention "enum".
No, it says "The rank of any enumerated type shall equal the rank of the
compatible integer type". For Emacs's enums, that type is 'int'.
>> So, when E is one of these enums, the type of +E (and of -E) is int.
>
> How many people you envision to be aware of the meaning of this
> trickery?
It's not trickery and everybody who counts will know it. You now know
it, I know it, and Emacs already uses it elsewhere.
If you prefer, we can package it inside a macro and put a comment on the
macro, to document it more explicitly. Something like this, perhaps:
/* The value of E, after the usual integer promotions.
This is safer than a cast or inline function,
as it does only integer promotions.
"switch (promote (E))" pacifies gcc -Wswitch-enum
when some enum values are deliberately omitted from the cases.
*/
#define promote(E) (+ (E))
> Wouldn't a cast (preferably followed by a comment) do the job?
Not as well, for reasons discussed above.
> There are no invalid values in that place, by definition.
Oh, I thought the code was worrying about something like uni-bidi.el
being corrupted and containing invalid data. If that sort of thing is
impossible then you're right, there should be no need for an extra
runtime test.
But in that case, why does bidi_get_type have a runtime test for
UNKNOWN_BT? Shouldn't that also be impossible?
It would help the reader to explain why UNKNOWN_BT needs to be checked
for, but non-enum values need not be. That could be put into the comment.
> The code
> checks for invalid bidi types elsewhere, where it needs that, by using
> bidi_check_type.
This patch was to bidi_check_type itself, not to its callers.
Do you mean that bidi_check_type's callers all check for invalid bidi
types (i.e., types not listed under bidi_type_t)? Can you give an
example of that? I'm not following the logic here.
>> In this example, pacifying -Wswitch-enum helped find code where Emacs's
>> internal runtime checking was missing some invalid values.
>
> Which code was that?
Currently the code does this:
default_type = (bidi_type_t) XFIXNUM (CHAR_TABLE_REF
(bidi_type_table, ch));
Suppose 'default_type' at this point has the value 100, because
uni-bidi.el was corrupted or something like that. Then the next statement:
if (default_type == UNKNOWN_BT)
emacs_abort ();
does not abort because UNKNOWN_BT is 0, not 100. And the following
'switch (default_type)' statement merely causes bidi_get_type to return
100, a value that is not in the bidi_type_t enum that bidi_get_type is
declared to return. Surely that is problematic.
>> - switch (prev_type)
>> + switch (+prev_type)
>> {
>> case RLI: /* X5a */
>
> "X5a" is a reference to a section in the UBA description, where the
> interested reader can find the details which will explain why only
> some values are mentioned. Why is that worse...?
It's not worse. "X5a" is a useful comment and should be left in, which
is what the patch did. There's no implication in the patch that "X5a" is
worse.
>> Remove ‘default: break;’ cases that are not now needed to pacify GCC.
>
> They were there not for pacifying GCC, they were an important part of
> the code.
I must confess that the comment in:
default:
/* Nothing. */
break;
conveyed no useful information to me because "default: break;" obviously
does nothing and is equivalent to omitting the default case entirely. So
to me, the comment read like the comment in "i++; /* Add one to I. */".
But if these "default: break;"s are important for style let's leave them
in. Proposed patch to master attached. This pacifies -Wswitch-enum
without attempting to deal with UNKNOWN_BT issue, and without removing
any "default: break;"s.
[0001-Pacify-gcc-Wswitch-enum-in-bidi.c.patch (text/x-patch, attachment)]
This bug report was last modified 127 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.