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

From: Paul Eggert <[email-masked]>
Date: Sun, 2 Feb 2025 10:27:02 -0800

On 2025-02-02 03:48, Pip Cet wrote:
> I've also stayed away from lib-src for now.

Although you did look at lib-src (I see some changes there) you 
understandably did not tackle lib-src/etags.c which has many enum 
switches and gimmicks to pacify GCC. So I took a crack at etags.c and 
came up with an alternative proposal consisting of two suggestions to 
support the following goals:

* Cleanly compile with -Wswitch-enum to catch potential errors.
* A style that is easy to explain and understand.
* No changes to GCC needed for now.
* No new C macros in the Emacs source code.
* Source code that is shorter and easier to maintain.

Here's the proposal:

Suggestion 1: Don't treat all enum switch statements the same. Some enum 
switch statements merely want to treat a couple of enum values 
specially, with all other enum values being default. For these, it's 
unlikely that forcing the programmer to list a case for every enum value 
will catch many bugs; it's even possible that this would cause more 
problems than it'll cure, and it's certainly an annoyance.

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 
is that if a switch statement has more than three "case X: break;"s then 
it may be a good idea to use "switch (+E)" instead of "switch (E)".

Suggestion 2: Omit "default: break;"s present only to tell GCC and/or 
the reader that the switch is otherwise not exhaustive. "switch (+expr)" 
already does this more concisely and more usefully.

Combine these two suggestions, and I made etags.c shorter (by 23 lines) 
and easier to read once you know the style. See attached patch, which 
compiles cleanly with -Wswitch-enum.

I assume similar results would apply elsewhere in Emacs.

From 8cbd3ce89a30e0968831bcb7b56edff031adbfd8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sun, 2 Feb 2025 10:01:46 -0800
Subject: [PATCH] Pacify gcc -Wswitch-enum in etags.c
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lib-src/etags.c (main, consider_token, C_entries)
(TeX_commands): For every switch (E) with an enum expression E,
either enumerate all the enum values as cases, or if that’s too
verbose 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.
---
 lib-src/etags.c | 85 ++++++++++++++++++-------------------------------
 1 file changed, 31 insertions(+), 54 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index b59b70c9ec7..619d0765867 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -1397,6 +1397,7 @@ main (int argc, char **argv)
           this_file = argbuffer[i].what;
           process_file (stdin, this_file, lang);
           break;
+	case at_end:
 	default:
 	  error ("internal error: arg_type");
 	}
@@ -1444,11 +1445,11 @@ main (int argc, char **argv)
 	    {
 	    case at_filename:
 	    case at_stdin:
+	      cleanup_tags_file (tagfile, argbuffer[i].what);
+	      break;
+	    case at_end: case at_language: case at_regexp:
 	      break;
-	    default:
-	      continue;		/* the for loop */
 	    }
-          cleanup_tags_file (tagfile, argbuffer[i].what);
 	}
       append_to_tagfile = true;
     }
@@ -3147,7 +3148,7 @@ consider_token (char *str,	      /* IN: token pointer */
 	 }
        break;
      case tkeyseen:
-       switch (toktype)
+       switch (+toktype)
 	 {
 	 case st_none:
 	 case st_C_class:
@@ -3155,8 +3156,6 @@ consider_token (char *str,	      /* IN: token pointer */
 	 case st_C_enum:
 	   typdef = ttypeseen;
 	   break;
-	 default:
-	   break;
 	 }
        break;
      case ttypeseen:
@@ -3167,7 +3166,7 @@ consider_token (char *str,	      /* IN: token pointer */
 	 }
        break;
      case tend:
-       switch (toktype)
+       switch (+toktype)
 	 {
 	 case st_C_class:
 	 case st_C_struct:
@@ -3176,11 +3175,11 @@ consider_token (char *str,	      /* IN: token pointer */
 	 default:
 	   return true;
 	 }
-     default:
+     case tinbody: case tignore:
        break;
      }
 
-   switch (toktype)
+   switch (+toktype)
      {
      case st_C_javastruct:
        if (structdef == stagseen)
@@ -3210,8 +3209,6 @@ consider_token (char *str,	      /* IN: token pointer */
 	     fvdef = fvnone;
 	 }
        return false;
-     default:
-       break;
      }
 
    if (structdef == skeyseen)
@@ -3227,7 +3224,7 @@ consider_token (char *str,	      /* IN: token pointer */
    switch (objdef)
      {
      case onone:
-       switch (toktype)
+       switch (+toktype)
 	 {
 	 case st_C_objprot:
 	   objdef = oprotocol;
@@ -3235,8 +3232,6 @@ consider_token (char *str,	      /* IN: token pointer */
 	 case st_C_objimpl:
 	   objdef = oimplementation;
 	   return false;
-	 default:
-	   break;
 	 }
        break;
      case oimplementation:
@@ -3296,16 +3291,16 @@ consider_token (char *str,	      /* IN: token pointer */
 	   objdef = onone;
 	 }
        return false;
-     default:
+     case ocatseen: case omethodtag: case otagseen:
        break;
      }
 
    /* A function, variable or enum constant? */
-   switch (toktype)
+   switch (+toktype)
      {
      case st_C_extern:
        fvextern = true;
-       switch  (fvdef)
+       switch (+fvdef)
 	 {
 	 case finlist:
 	 case flistseen:
@@ -3332,7 +3327,7 @@ consider_token (char *str,	      /* IN: token pointer */
 	      constants.  */
 	   && fvdef != vignore)
 	 return true;		/* enum constant */
-       switch (fvdef)
+       switch (+fvdef)
 	 {
 	 case fdefunkey:
 	   if (bracelev > 0)
@@ -3341,7 +3336,7 @@ consider_token (char *str,	      /* IN: token pointer */
 	   *is_func_or_var = true;
 	   return true;
 	 case fvnone:
-	   switch (typdef)
+	   switch (+typdef)
 	     {
 	     case ttypeseen:
 	       return false;
@@ -3353,8 +3348,6 @@ consider_token (char *str,	      /* IN: token pointer */
 		   return false;
 		 }
 	       break;
-	     default:
-	       break;
 	     }
 	   FALLTHROUGH;
 	  case fvnameseen:
@@ -3371,12 +3364,8 @@ consider_token (char *str,	      /* IN: token pointer */
 	  fvdef = fvnameseen;	/* function or variable */
 	  *is_func_or_var = true;
 	  return true;
-	 default:
-	   break;
 	}
       break;
-     default:
-       break;
     }
 
   return false;
@@ -3581,7 +3570,7 @@ C_entries (int c_ext,		/* extension of C */
 	    continue;
 	  if (inattribute)
 	    break;
-	  switch (fvdef)
+	  switch (+fvdef)
 	    {
 	    case fdefunkey:
 	    case fstartlist:
@@ -3883,7 +3872,7 @@ C_entries (int c_ext,		/* extension of C */
 	      switch (definedef)
 		{
 		case dnone:
-		  switch (fvdef)
+		  switch (+fvdef)
 		    {
 		    case fstartlist:
 		      /* This prevents tagging fb in
@@ -3898,8 +3887,6 @@ C_entries (int c_ext,		/* extension of C */
 			  fvdef = fignore;
 			}
 		      break;
-		    default:
-		      break;
 		    }
 		  if (structdef == stagseen && !cjava)
 		    {
@@ -3910,7 +3897,7 @@ C_entries (int c_ext,		/* extension of C */
 		case dsharpseen:
 		  savetoken = token;
 		  break;
-		default:
+		case ddefineseen: case dignorerest:
 		  break;
 		}
 	      if (!yacc_rules || lp == newlb.buffer + 1)
@@ -3938,7 +3925,7 @@ C_entries (int c_ext,		/* extension of C */
 	    }
 	  if (definedef != dnone)
 	    break;
-	  switch (objdef)
+	  switch (+objdef)
 	    {
 	    case otagseen:
 	      objdef = oignore;
@@ -3954,8 +3941,6 @@ C_entries (int c_ext,		/* extension of C */
 		  strcpy (token_name.buffer + toklen, ":");
 		}
 	      break;
-	    default:
-	      break;
 	    }
 	  if (structdef == stagseen)
 	    {
@@ -3984,7 +3969,7 @@ C_entries (int c_ext,		/* extension of C */
 	    case tnone:
 	    case tinbody:
 	    case tignore:
-	      switch (fvdef)
+	      switch (+fvdef)
 		{
 		case fignore:
 		  if (typdef == tignore || cplpl)
@@ -4016,6 +4001,7 @@ C_entries (int c_ext,		/* extension of C */
 		    token.valid = false;
 		} /* switch (fvdef) */
 	      FALLTHROUGH;
+	    case tkeyseen:
 	    default:
 	      if (!instruct)
 		typdef = tnone;
@@ -4026,15 +4012,13 @@ C_entries (int c_ext,		/* extension of C */
 	case ',':
 	  if (definedef != dnone || inattribute)
 	    break;
-	  switch (objdef)
+	  switch (+objdef)
 	    {
 	    case omethodtag:
 	    case omethodparm:
 	      make_C_tag (true); /* an Objective C method */
 	      objdef = oinbody;
 	      break;
-	    default:
-	      break;
 	    }
 	  switch (fvdef)
 	    {
@@ -4071,6 +4055,7 @@ C_entries (int c_ext,		/* extension of C */
 		fvdef = fvnone;
 	      token.valid = false;
 	      break;
+	    case fvnone:
 	    default:
 	      fvdef = fvnone;
 	    }
@@ -4093,7 +4078,7 @@ C_entries (int c_ext,		/* extension of C */
 	      break;
 	    case tnone:
 	    case tinbody:
-	      switch (fvdef)
+	      switch (+fvdef)
 		{
 		case foperator:
 		case finlist:
@@ -4110,7 +4095,7 @@ C_entries (int c_ext,		/* extension of C */
 		  fvdef = fvnone;
 		}
 	      break;
-	    default:
+	    case tignore: case tkeyseen:
 	      break;
 	    }
 	  break;
@@ -4124,7 +4109,7 @@ C_entries (int c_ext,		/* extension of C */
 	    break;
 	  if (objdef == otagseen && parlev == 0)
 	    objdef = oparenseen;
-	  switch (fvdef)
+	  switch (+fvdef)
 	    {
 	    case fvnameseen:
 	      if (typdef == ttypeseen
@@ -4145,8 +4130,6 @@ C_entries (int c_ext,		/* extension of C */
 	    case flistseen:
 	      fvdef = finlist;
 	      break;
-	    default:
-	      break;
 	    }
 	  parlev++;
 	  break;
@@ -4172,14 +4155,12 @@ C_entries (int c_ext,		/* extension of C */
 	    }
 	  if (--parlev == 0)
 	    {
-	      switch (fvdef)
+	      switch (+fvdef)
 		{
 		case fstartlist:
 		case finlist:
 		  fvdef = flistseen;
 		  break;
-		default:
-		  break;
 		}
 	      if (!instruct
 		  && (typdef == tend
@@ -4202,7 +4183,7 @@ C_entries (int c_ext,		/* extension of C */
 	      typdef = tinbody;
 	      typdefbracelev = bracelev;
 	    }
-	  switch (fvdef)
+	  switch (+fvdef)
 	    {
 	    case flistseen:
 	      if (cplpl && !class_qualify)
@@ -4233,7 +4214,7 @@ C_entries (int c_ext,		/* extension of C */
 	      fvdef = fvnone;
 	      break;
 	    case fvnone:
-	      switch (objdef)
+	      switch (+objdef)
 		{
 		case otagseen:
 		  make_C_tag (true); /* an Objective C class */
@@ -4251,8 +4232,6 @@ C_entries (int c_ext,		/* extension of C */
 		    bracelev = -1;
 		}
 	      break;
-	    default:
-	      break;
 	    }
 	  switch (structdef)
 	    {
@@ -4266,7 +4245,7 @@ C_entries (int c_ext,		/* extension of C */
 	      structdef = snone;
 	      make_C_tag (false);  /* a struct or enum */
 	      break;
-	    default:
+	    case snone:
 	      break;
 	    }
 	  bracelev += 1;
@@ -4316,7 +4295,7 @@ C_entries (int c_ext,		/* extension of C */
 	case '=':
 	  if (definedef != dnone)
 	    break;
-	  switch (fvdef)
+	  switch (+fvdef)
 	    {
 	    case foperator:
 	    case finlist:
@@ -4361,7 +4340,7 @@ C_entries (int c_ext,		/* extension of C */
 	  if (definedef != dnone)
 	    break;
 	  /* These surely cannot follow a function tag in C. */
-	  switch (fvdef)
+	  switch (+fvdef)
 	    {
 	    case foperator:
 	    case finlist:
@@ -6006,8 +5985,6 @@ TeX_commands (FILE *inf)
 				goto tex_next_line;
 			  }
 			break;
-		      default:
-			break;
 		      }
 		  }
 		namelen = p - cp;
-- 
2.45.2