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

Previous Next

Full log

Message #212 received at 75964 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Wed, 05 Feb 2025 18:10:16 +0000

"Paul Eggert" <eggert <at> cs.ucla.edu> writes:

> On 2/3/25 04:08, Eli Zaretskii wrote:
>>> From: Paul Eggert <eggert <at> cs.ucla.edu>
>>> The main point is to catch more potential inadvertent omissions of "case
>>> X:" in a "switch (E)" where E's type is an enum containing X. etags.c,
>>> like the rest of Emacs, currently uses one style to do this; the
>>> proposed style would be better at catching these mistakes.
>>
>> And how do the proposed changes achieve that goal?
>
> With an enum expression E and code like this:
>
>     switch (E) { case A: ...; case B: ...; }
>
> there is currently no compile-time diagnostic for mismatches between the
> type and the code when the enum type also has a value C, either because
> the programmer forgot to handle case C when the switch was written, or
> because the enum type was changed and the programmer forgot to check all
> uses of the enum.

Sorry this got long.  I think it's worth it to do this, but I don't
think a redundant + operator is a good way to avoid the warning, sorry.

I'm very confused here.  If you mean code like this:

enum ABC { A, B, C, };

int sw(enum ABC abc)
{
  switch (abc)
    {
    case A: return 1;
    case B: return 2;
    }
}

gcc -Wswitch will warn about it.


If you mean code like this:

enum ABC { A, B, C, };

int sw(enum ABC abc)
{
  switch (abc)
    {
    case A: return 1;
    case B: return 2;
    default: return -1;
    }
}

gcc -Wswitch-enum will warn about it, but gcc -Wswitch won't.

> Mismatches like these are all too common and can lead to user-visible
> bugs. It's a win to detect these mismatches statically.

The problem is that we lose the ability to abbreviate switch statements
where we know "default" covers both enumerated cases and non-enumerated
ones.  I thought this was rare enough to avoid warning about those
switches using a #pragma, but it's more common than that.

The current GCC behavior of treating a switch statement as
non-enumerated if the control expression isn't of enum type leads to
false negatives, and I'd like to change it.  Relying on it as a way to
disable warnings using the switch (+x) idiom seems unwise to me for that
reason, because we will then permanently lose those important warnings.

For example, there are currently two instances of switch (XFIXNUM (...))
in my Emacs tree, and I'm responsible for only one of them.

>>> For these switch statements, use "switch (+E)" instead of "switch (E)".
>>> This pacifies GCC and clearly signals to the reader that the switch's
>>> cases are not intended to exhaust the enum. A "switch (E)" must list all
>>> the enum values; a "switch (+E)" need not do so. A reasonable guideline

I'm not sure whether you're using "+e" (lower-case, because it's a
variable, right?) as an idiom to be recognized by the compiler here or
because the compiler will assign it to a different type from e.  While
the compiler must generate the same code for switch ((int)e) and switch
(+e) (and switch (e) except in the most unlikely of cases), there is no
requirement for it to generate the same warnings.

So we shouldn't let C type coercion rules guide us here.

> Yes, and that's a plus for +E: you don't have to change the code much.
> In that sense it's better than Pip Cet's earlier proposal.

I agree that adding a back-door to get back to abbreviated switch
statements where "default" covers enumerated cases is a good thing.
(Well, I don't, see below; I just think resistance to -Wswitch-enum in
the absence of a readable and useful back-door is too great to be worth
it).

But switch (+e) is not a good one, not least because it appears at the
very beginning of the switch statement, rather than near the default
label where it's useful to the human reader.

The situation is analogous to the now-required fall-through labelling of
switch statements? That also happens near the label, and an initial hack
could just treat a fall-through default: statement as covering more enum
cases, while one without a fall-through is limited to the non-enum
cases...

In fact, given that we almost always want to have a default case which
is eassume(0), it may make more sense to explicitly enable -Wswitch-enum
warnings by marking the default label as applying only to the
"paradoxical" case that the enum variable's value isn't in the enum.

switch (e)
  {
  case A:
  case B:
    return 1;
  default __attribute__((impossible)):
    eassume (0);
  }

would warn, while

switch (e)
  {
  case A:
  case B:
    return 1;
  default:
    return 2;
  }

wouldn't.

This clearly needs more thought, particularly since the support for
label attributes in GCC isn't as nice as it could be.  I don't know the
GCC source code well enough to come up with a quick hack here.

But since macros are also considered bad form (how else are we supposed
to use GCC attributes?), I don't know how to do it at all.

Of course if all we care about is the occasional forgotten case, the
easy solution is to have a threshold heuristic: count the explicit cases
C and the possible cases N, and warn if C > N - 4 && C > 3 && C < N :-)

>>> +      case NEUTRAL_ON: case NEUTRAL_S: case NEUTRAL_WS:
>>> +      case STRONG_AL: case STRONG_L: case STRONG_R:
>>> +      case WEAK_AN: case WEAK_CS: case WEAK_EN: case WEAK_ES: case WEAK_ET: case WEAK_NSM:
>>>  	if (override == L2R)
>>>  	  return STRONG_L;
>>>  	else if (override == R2L)

As a very tangential remark, one of the great problems with simply
enabling -Wswitch-enum is that someone might list extra case labels in
random order, or even several unrelated ones on one line.  That appears
to have happened here.

Pip

Previous Next

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