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

From: Paul Eggert <[email-masked]>
Date: Sun, 2 Feb 2025 13:43:40 -0800

On 2025-02-02 11:27, Eli Zaretskii wrote:

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

Although the proposed style is shorter than what Emacs currently uses, 
brevity is not the main goal here.

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

If we use this style consistently it will be obvious, and gcc 
-Wswitch-enum will report inadvertent deviations from it which will be 
better than what we have now (no static checking). Also, developers who 
don't like the style won't be obliged to use it: they can use other 
techniques (already discussed) to pacify -Wswitch-enum if they prefer.

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

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

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.

All in all it's a success story.

From 400f3d8eb6d171f3a5692554ce2df59ebd3e1c5a Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sun, 2 Feb 2025 13:36:35 -0800
Subject: [PATCH] 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 (bidi_get_type): Abort if the default type has a
value not listed in the enum, instead of silently treating
it like NEUTRAL_ON etc.  List all enum values in a switch.
(bidi_resolve_explicit, bidi_find_bracket_pairs)
(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 | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/src/bidi.c b/src/bidi.c
index d8754e2db73..5656b7a9cf9 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,7 +297,18 @@ bidi_get_type (int ch, bidi_dir_t override)
       case FSI:
       case PDI:
 	return default_type;
+
+      case UNKNOWN_BT:
       default:
+	/* 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) or some unknown code from CHAR_TABLE_REF, that's a bug.  */
+	emacs_abort ();
+
+      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)
@@ -2010,7 +2015,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
@@ -2074,7 +2079,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 */
@@ -2197,9 +2202,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;
@@ -2707,7 +2709,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
@@ -2723,8 +2725,6 @@ bidi_find_bracket_pairs (struct bidi_it *bidi_it)
 			  : FLAG_OPPOSITE_INSIDE);
 		  r2l_seen = true;
 		  break;
-		default:
-		  break;
 		}
 	      if (flag)
 		{
@@ -2979,7 +2979,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:
@@ -2995,9 +2995,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);
@@ -3175,7 +3173,7 @@ bidi_resolve_neutral (struct bidi_it *bidi_it)
 	    }
 	  else
 	    {
-	      switch (type)
+	      switch (+type)
 		{
 		case STRONG_L:
 		case STRONG_R:
-- 
2.45.2