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

From: Eli Zaretskii <[email-masked]>
Date: Mon, 03 Feb 2025 14:08:30 +0200

> Date: Sun, 2 Feb 2025 13:43:40 -0800
> Cc: [email-masked], [email-masked], [email-masked]
> From: Paul Eggert <[email-masked]>
> 
> > What are the problems in etags.c this tries to solve?
> 
> 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?

> > "switch (+expr)" has the disadvantage that it is not widely known.
> 
> It is a simple combination of two widely-known constructs, "switch (E)" 
> and "+E". It is not a GNU extension: support for this has always been 
> required by the C standard and it works everywhere.

I am asking about the "+E" part.  You said in an earlier message:

> 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

The above seems to say that "+E" is different from "E" in ways that
the compiler knows about.  So I'm asking where is this special meaning
documented, to make sure I completely understand its semantics and its
treatment by the compiler.

> > Why would we want to use a construct that people
> > might not be familiar with?
> 
> Because when we add -Wswitch-enum, GCC will be more likely to catch 
> trivial mistakes that it doesn't currently catch.

If the special meaning of "+E" is not well understood, using it might
cause people make more mistakes.

> > I'm not sure shortening etags.c by 23 lines justifies this.
> 
> The main goal here is reliability via better static checking, not 
> brevity. Pip Cet's proposal to improve reliability makes the code more 
> verbose, which is a minus. This proposal does not have that minus - on 
> the contrary, it makes code shorter and simpler.

I'm not comparing this with what Pip Cet proposed, I compare it with
what we have now.

> To test this again, I applied the style to the file that Pip Cet said 
> was a troublesome case: src/bidi.c. Here the style made the code only a 
> little bit shorter, but that was because I took the liberty of also 
> improving the already-existing dynamic checking, as I modified 
> bidi_get_type to also abort if default_value is not listed in the 
> bidi_type_t enum. So this patch improves dynamic checking (without 
> significant runtime cost), improves static checking, and makes the code 
> a bit shorter and easier to read.

I don't think I see how these changes improve checking and make the
code easier to read.  For me, the "+E" thing is an obstacle to
negotiate; I'm sure others will also stumble on that.  And apart of
that change, the code basically remained the same.
> +      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)

If you really consider this long list of values more readable than the
original code, then I guess we disagree on what is and isn't readable.
As I mentioned up-thread, such a long list doesn't convince me that
all the values were mentioned.  So for me this is a net loss, because
the original code left no doubt that all the types get the
directionality-override handling.