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

From: Paul Eggert <[email-masked]>
Date: Tue, 4 Feb 2025 13:23:25 -0800

On 2/3/25 04:08, Eli Zaretskii wrote:
>> From: Paul Eggert <[email-masked]>
>> 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.

There *is* a compile-time diagnostic for similar mismatches between type 
and code, e.g., if the enum type lacks a B (perhaps because B was 
formerly present but has since been removed). But there is no 
compile-time diagnostic for the mismatch in question.

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

>> 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

It is more than merely documented; it's been common practice for years. 
For example, given enum W {X,Y,Z} the type of an expression like X+1 has 
been int (not enum W) ever since enums were standardized in the C89 
standard.

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. Also please see C23 §6.3.1.1 ¶1–2, which says that 
int is the type of these Emacs enums after integer promotion. So, when E 
is one of these enums, the type of +E (and of -E) is int.

> If the special meaning of "+E" is not well understood

I hope the above suffices to explain why it is well understood.

> 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.

If necessary we can package "+E" inside an inline identity function. 
That might help prevent stumbling by people who don't know the C 
language well, and it would not have any runtime cost in the usual -O2 
case. I'd rather not use such a function (as +E is clear once you get 
used to it) but using one would be better than doing nothing, as it 
would improve static checking.

> apart of that change, the code basically remained the same.

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.

>> +      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.

Oh, that long list was in some sense a misfire, as the patch did two 
things where I suppose it should have done one. First, the patch 
improved the runtime checkcing for invalid values, something that can 
and should be done independently of what we do about -Wswitch-enum. 
Second, the patch pacified -Wswitch-enum.

I fixed this misfire by splitting the patch into two parts, which I'm 
attaching. The first attached patch improves the runtime checking 
without changing the -Wswitch-enum style, and I installed that on 
master. The second attached patch, which I have not installed, is the 
-Wswitch-enum change proper.

In this example, pacifying -Wswitch-enum helped find code where Emacs's 
internal runtime checking was missing some invalid values. So this is an 
example of why this style change is helpful in practice.

From 782ec71053d8535511522f27f28c11682ca0f40b Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 4 Feb 2025 11:43:58 -0800
Subject: [PATCH 1/2] Improve bidi_get_time runtime checking

* src/bidi.c (bidi_get_type): Improve runtime checking, by also
aborting if the bidi_type_table entry is not a bidi_type_t value.
---
 src/bidi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/bidi.c b/src/bidi.c
index d8754e2db73..fd0bebb85e0 100644
--- a/src/bidi.c
+++ b/src/bidi.c
@@ -282,12 +282,6 @@ bidi_get_type (int ch, bidi_dir_t override)
     emacs_abort ();
 
   default_type = (bidi_type_t) XFIXNUM (CHAR_TABLE_REF (bidi_type_table, ch));
-  /* Every valid character code, even those that are unassigned by the
-     UCD, have some bidi-class property, according to
-     DerivedBidiClass.txt file.  Therefore, if we ever get UNKNOWN_BT
-     (= zero) code from CHAR_TABLE_REF, that's a bug.  */
-  if (default_type == UNKNOWN_BT)
-    emacs_abort ();
 
   switch (default_type)
     {
@@ -303,13 +297,26 @@ bidi_get_type (int ch, bidi_dir_t override)
       case FSI:
       case PDI:
 	return default_type;
-      default:
+
+      case STRONG_L: case STRONG_R:
+      case WEAK_EN: case WEAK_AN:
+      case STRONG_AL:
+      case WEAK_ES: case WEAK_ET: case WEAK_CS: case WEAK_NSM:
+      case NEUTRAL_S: case NEUTRAL_WS: case NEUTRAL_ON:
 	if (override == L2R)
 	  return STRONG_L;
 	else if (override == R2L)
 	  return STRONG_R;
 	else
 	  return default_type;
+
+      case UNKNOWN_BT:
+      default:
+	/* Every valid character code, even those unassigned by the UCD,
+	   have some bidi-class property, according to DerivedBidiClass.txt.
+	   Therefore, if we ever get UNKNOWN_BT (= zero) or some unknown
+	   code from CHAR_TABLE_REF, that's a bug.  */
+	emacs_abort ();
     }
 }
 
-- 
2.48.1

From e75ff04e92d16394dcd7b24a87adf1e2fc5bae91 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 4 Feb 2025 13:02:18 -0800
Subject: [PATCH 2/2] Pacify gcc -Wswitch-enum in bidi.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/bidi.c (characters, bidi_find_bracket_pairs)
(bidi_resolve_brackets, bidi_resolve_neutral):
Use ‘switch (+E)’ to indicate that it’s intended that we not
enumerate all the enum values.
Remove ‘default: break;’ cases that are not now needed to pacify GCC.
---
 src/bidi.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/bidi.c b/src/bidi.c
index fd0bebb85e0..fae5f04d03d 100644
--- a/src/bidi.c
+++ b/src/bidi.c
@@ -2017,7 +2017,7 @@ bidi_resolve_explicit (struct bidi_it *bidi_it)
 	 embedding level of the _following_ characters, so we must
 	 first look at the type of the previous character to support
 	 that.  */
-      switch (prev_type)
+      switch (+prev_type)
 	{
 	case RLI:	/* X5a */
 	  if (current_level < BIDI_MAXDEPTH
@@ -2081,7 +2081,7 @@ bidi_resolve_explicit (struct bidi_it *bidi_it)
 
   bidi_it->type_after_wn = UNKNOWN_BT;
 
-  switch (type)
+  switch (+type)
     {
     case RLE:	/* X2 */
     case RLO:	/* X4 */
@@ -2204,9 +2204,6 @@ bidi_resolve_explicit (struct bidi_it *bidi_it)
       bidi_check_type (bidi_it->type_after_wn);
       type = WEAK_BN; /* X9/Retaining */
       break;
-    default:
-      /* Nothing.  */
-      break;
     }
 
   bidi_it->type = type;
@@ -2714,7 +2711,7 @@ bidi_find_bracket_pairs (struct bidi_it *bidi_it)
 
 	      /* Whenever we see a strong type, update the flags of
 		 all the slots on the stack.  */
-	      switch (bidi_it->type)
+	      switch (+bidi_it->type)
 		{
 		case STRONG_L:
 		  flag = ((embedding_level & 1) == 0
@@ -2730,8 +2727,6 @@ bidi_find_bracket_pairs (struct bidi_it *bidi_it)
 			  : FLAG_OPPOSITE_INSIDE);
 		  r2l_seen = true;
 		  break;
-		default:
-		  break;
 		}
 	      if (flag)
 		{
@@ -2986,7 +2981,7 @@ bidi_resolve_brackets (struct bidi_it *bidi_it)
 
 	  if (prev_type_for_neutral == UNKNOWN_BT)
 	    prev_type_for_neutral = embedding_type;
-	  switch (prev_type_for_neutral)
+	  switch (+prev_type_for_neutral)
 	    {
 	    case STRONG_R:
 	    case WEAK_EN:
@@ -3002,9 +2997,7 @@ bidi_resolve_brackets (struct bidi_it *bidi_it)
 		? STRONG_L				     /* N0c1 */
 		: embedding_type;			     /* N0c2 */
 	      break;
-	    default:
 	      /* N0d: Do not set the type for that bracket pair.  */
-	      break;
 	    }
 	}
       eassert (type == STRONG_L || type == STRONG_R || type == NEUTRAL_ON);
@@ -3182,7 +3175,7 @@ bidi_resolve_neutral (struct bidi_it *bidi_it)
 	    }
 	  else
 	    {
-	      switch (type)
+	      switch (+type)
 		{
 		case STRONG_L:
 		case STRONG_R:
-- 
2.48.1