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

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Andrea Corallo <acorallo <at> gnu.org>, Eli Zaretskii <eliz <at> gnu.org>, eggert <at> cs.ucla.edu, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Mon, 03 Feb 2025 09:01:40 +0000

"Stefan Monnier" <monnier <at> iro.umontreal.ca> writes:

>> 	      switch (bidi_it->type)
>> 		{
>> 		case STRONG_L:
>> 		  flag = ((embedding_level & 1) == 0
>> 			  ? FLAG_EMBEDDING_INSIDE
>> 			  : FLAG_OPPOSITE_INSIDE);
>> 		  l2r_seen = true;
>> 		  break;
>> 		case STRONG_R:
>> 		case WEAK_EN:
>> 		case WEAK_AN:
>> 		  flag = ((embedding_level & 1) == 1
>> 			  ? FLAG_EMBEDDING_INSIDE
>> 			  : FLAG_OPPOSITE_INSIDE);
>> 		  r2l_seen = true;
>> 		  break;
>> 		case UNKNOWN_BT:
>> 		case WEAK_BN:
>> 		case NEUTRAL_B:
>> 		case STRONG_AL:
>> 		case LRE:
>> 		case LRO:
>> 		case RLE:
>> 		case RLO:
>> 		case PDF:
>> 		case LRI:
>> 		case RLI:
>> 		case FSI:
>> 		case PDI:
>> 		case WEAK_ES:
>> 		case WEAK_ET:
>> 		case WEAK_CS:
>> 		case WEAK_NSM:
>> 		case NEUTRAL_S:
>> 		case NEUTRAL_WS:
>> 		case NEUTRAL_ON:
>> 		default: 		  break;
>> 		}
>>
>> I think that could be rewritten to be more readable in a number of ways,
>
> The most obvious one is to remove all the `case ..:` just before `default:`.
> That would be my vote as well (tho only because I don't know that
> code.  There might be a very good reason to keep it as above, of course).

The reason was to add -Wswitch-enum, which would warn about incomplete
case enumerations involving an enum type.  My hope was to make some
existing bugs more obvious that way, because applying a patch like:

@@ -26086,8 +26124,9 @@ x_draw_window_cursor (struct window *w, struct glyph_row *glyph_row, int x,
 	      w->phys_cursor_width = 0;
 	      break;
 
-	    default:
+	    case DEFAULT_CURSOR:
 	      emacs_abort ();
+	    default: 	      emacs_abort ();
 	    }
 	}


does make you wonder what's so special about the DEFAULT_CURSOR case
that aborting when we're asked to draw one is the right thing to do.

(There's probably a good reason in this case).

There are more cases like this in the patch I sent.  I guess the
consensus now is not to add -Wswitch-enum to regular builds at all, so
we'll just have to live with those bugs.

>> including, of course, putting a #pragma in.
>
> I hate `#pragma`.

Consider it a failed last-ditch attempt to enable these warnings on a
per-file basis, by using #include "exhaustive.h" or a similar construct.

We still have the option of doing so in Makefile.in, but who reads
Makeile.in?

So I guess we'll just do nothing.

Pip

Previous Next

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