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

From: Paul Eggert <[email-masked]>
Date: Wed, 5 Feb 2025 11:20:47 -0800

On 2025-02-05 05:05, Eli Zaretskii wrote:

> There's no way to know, just by looking at the code, whether the
> omission is a mistake or deliberate and required.

Which is exactly why these sorts of patches, if done right, are a good 
idea. They make it clear to the compiler and the reader whether these 
omission are deliberate.

> I very much doubt these techniques will make our situation
> significantly better, if at all.

Admittedly Emacs has more serious bugs than enum mismatches. However, 
checking these mismatches can help find mistakes or help clarify code.

> So basically the plus sign converts the enum to an int, and thus shuts
> up the compiler warnings?  If so, why not use an explicit cast

A cast like ((int) E) is too powerful in C. For example, it works even 
if E is a pointer, or is floating point. That would introduce more 
possibilities for mistakes than +E does. When possible (as is the case 
here) we should avoid using casts.

>> 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.
> 
> And which doesn't mention "enum" even once.

It says that the integer promotions are performed. The Emacs enums 
promote to int, as shown below.

>> Also please see C23 §6.3.1.1 ¶1–2, which says that
>> int is the type of these Emacs enums after integer promotion.
> 
> Which also don't mention "enum".

No, it says "The rank of any enumerated type shall equal the rank of the 
compatible integer type". For Emacs's enums, that type is 'int'.

>> So, when E is one of these enums, the type of +E (and of -E) is int.
> 
> How many people you envision to be aware of the meaning of this
> trickery?

It's not trickery and everybody who counts will know it. You now know 
it, I know it, and Emacs already uses it elsewhere.

If you prefer, we can package it inside a macro and put a comment on the 
macro, to document it more explicitly. Something like this, perhaps:

    /* The value of E, after the usual integer promotions.
       This is safer than a cast or inline function,
       as it does only integer promotions.
       "switch (promote (E))" pacifies gcc -Wswitch-enum
       when some enum values are deliberately omitted from the cases. 
*/
    #define promote(E) (+ (E))

> Wouldn't a cast (preferably followed by a comment) do the job?

Not as well, for reasons discussed above.

> There are no invalid values in that place, by definition.

Oh, I thought the code was worrying about something like uni-bidi.el 
being corrupted and containing invalid data. If that sort of thing is 
impossible then you're right, there should be no need for an extra 
runtime test.

But in that case, why does bidi_get_type have a runtime test for 
UNKNOWN_BT? Shouldn't that also be impossible?

It would help the reader to explain why UNKNOWN_BT needs to be checked 
for, but non-enum values need not be. That could be put into the comment.

> The code
> checks for invalid bidi types elsewhere, where it needs that, by using
> bidi_check_type.

This patch was to bidi_check_type itself, not to its callers.

Do you mean that bidi_check_type's callers all check for invalid bidi 
types (i.e., types not listed under bidi_type_t)? Can you give an 
example of that? I'm not following the logic here.

>> In this example, pacifying -Wswitch-enum helped find code where Emacs's
>> internal runtime checking was missing some invalid values.
> 
> Which code was that?

Currently the code does this:

   default_type = (bidi_type_t) XFIXNUM (CHAR_TABLE_REF 
(bidi_type_table, ch));

Suppose 'default_type' at this point has the value 100, because 
uni-bidi.el was corrupted or something like that. Then the next statement:

   if (default_type == UNKNOWN_BT)
     emacs_abort ();

does not abort because UNKNOWN_BT is 0, not 100. And the following 
'switch (default_type)' statement merely causes bidi_get_type to return 
100, a value that is not in the bidi_type_t enum that bidi_get_type is 
declared to return. Surely that is problematic.

>> -      switch (prev_type)
>> +      switch (+prev_type)
>>   	{
>>   	case RLI:	/* X5a */
> 
> "X5a" is a reference to a section in the UBA description, where the
> interested reader can find the details which will explain why only
> some values are mentioned.  Why is that worse...?

It's not worse. "X5a" is a useful comment and should be left in, which 
is what the patch did. There's no implication in the patch that "X5a" is 
worse.

>> Remove ‘default: break;’ cases that are not now needed to pacify GCC.
> 
> They were there not for pacifying GCC, they were an important part of
> the code.

I must confess that the comment in:

     default:
       /* Nothing.  */
       break;

conveyed no useful information to me because "default: break;" obviously 
does nothing and is equivalent to omitting the default case entirely. So 
to me, the comment read like the comment in "i++; /* Add one to I.  */".

But if these "default: break;"s are important for style let's leave them 
in. Proposed patch to master attached. This pacifies -Wswitch-enum 
without attempting to deal with UNKNOWN_BT issue, and without removing 
any "default: break;"s.

From 6e4f1f628689aa1e38ca112adc7eeb0400ce3e78 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 5 Feb 2025 11:06:40 -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, bidi_resolve_explicit)
(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.
---
 src/bidi.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/bidi.c b/src/bidi.c
index d8754e2db73..2c0a09d6210 100644
--- a/src/bidi.c
+++ b/src/bidi.c
@@ -289,7 +289,7 @@ bidi_get_type (int ch, bidi_dir_t override)
   if (default_type == UNKNOWN_BT)
     emacs_abort ();
 
-  switch (default_type)
+  switch (+default_type)
     {
       case WEAK_BN:
       case NEUTRAL_B:
@@ -2010,7 +2010,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 +2074,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 */
@@ -2707,7 +2707,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
@@ -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:
@@ -3175,7 +3175,7 @@ bidi_resolve_neutral (struct bidi_it *bidi_it)
 	    }
 	  else
 	    {
-	      switch (type)
+	      switch (+type)
 		{
 		case STRONG_L:
 		case STRONG_R:
-- 
2.45.2