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

From: Paul Eggert <[email-masked]>
Date: Sat, 8 Feb 2025 19:40:21 -0800

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

> "Promote" is too general, IMO, and will still be a riddle.  Something
> like 'to_int' (with the commentary you propose) could be better,
> because it says more clearly what is the intent here.

OK, the attcahed bidi.c patch uses the more-explicit name "INT_PROMOTE" 
instead of just "promote", with expanded commentary in the definition of 
INT_PROMOTE. The word "promote" is helpful because it emphasizes that 
the macro does only integer promotions and therefore does not affect the 
value, and that it does not do the implicit or explicit conversions that 
a name like "to_int" would suggest (such conversions could affect the 
value which of course would be a bigger deal for a switch).

Because I added INT_PROMOTE to Gnulib's intprops.h (a natural home for 
it) and because Emacs has already imported the new intprops.h, the only 
changes needed are to some of bidi.c's switch expressions.

>> Oh, I thought the code was worrying about something like uni-bidi.el
>> being corrupted and containing invalid data.
> 
> If we want to take into account the possibility of corrupted code,
> then all bets are off,

Yes, but that's why I was puzzled by the "if (default_type == 
UNKNOWN_BT)" test in src/bidi.c line 289. I couldn't see why that test 
was needed if we assume that uni-bidi.el etc. are not corrupted.

>> But in that case, why does bidi_get_type have a runtime test for
>> UNKNOWN_BT? Shouldn't that also be impossible?
> 
> getting UNKNOWN_BT there could mean one of the
> following:
> 
>   . the UnicodeData.txt file used for building Emacs was corrupted or
>     incorrectly interpreted by our scripts
>   . admin/unidata-gen.el has a bug or was incorrectly adjusted to
>     changes in Unicode
>   . there's a bug in chartab.c code that deals with uniprop char-tables

Yes, but if any of those things happen, couldn't one also get (say) -1 
or 100 instead of 0? That is, one could get any value outside the range 
STRONG_L..NEUTRAL_ON, and if so it might be helpful (and almost as 
cheap) to check for all those bogus values, not for just 0.

> search bidi.c for the calls to bidi_check_type.  You will see
> that bidi_check_type is called in many places where some function can
> return a bidi_type_t value, including after calling bidi_get_type, to
> make sure we don't get values outside of the enumeration.

OK, but many calls to bidi_get_type are not followed by bidi_check_type 
calls, so they don't get the benefit of bidi_check_type's eassert. The 
first two calls to bidi_check_type that textually occur in bidi.c (both 
from bidi_fetch_char_skip_isolates) are like that, for example. Wouldn't 
these calls benefit from the extra check?

> if we want to call bidi_check_type (or something
> similar) right after CHAR_TABLE_REF, I won't mind (but then we'd need
> to abort, not eassert, and several calls to bidi_check_type elsewhere
> in the file will become redundant).

Totally up to you of course what sort of behavior is appropriate if a 
failure is discovered.

> why make the code harder to read by adding those unary plus signs
> where the code and the references to the UBA should clearly explain
> the intent?  What does the unary plus sign gain us?

It improves GCC's static checking for common errors involving enum switches.

The change is not primarily about readability for each individual case. 
That being said, it can help the human reader by giving the human an 
easy-to-see visual cue that this 'switch' statement deliberately omits 
some enum cases. It can often be helpful to see this up front than to 
have to painstakingly determine it by looking at all the cases and 
looking at the enum's definition and comparing the two sets of values.

From 4148b52a86eb6f2f0b7ddae5110a5f97f23fcb7a Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Sat, 8 Feb 2025 15:56:04 -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 (INT_PROMOTE (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..773513143d6 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 (INT_PROMOTE (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 (INT_PROMOTE (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 (INT_PROMOTE (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 (INT_PROMOTE (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 (INT_PROMOTE (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 (INT_PROMOTE (type))
 		{
 		case STRONG_L:
 		case STRONG_R:
-- 
2.45.2