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

Previous Next

Package: emacs;

Reported by: Pip Cet <pipcet <at> protonmail.com>

Date: Fri, 31 Jan 2025 09:41:02 UTC

Severity: wishlist

Full log


View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: acorallo <at> gnu.org, eggert <at> cs.ucla.edu, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com, monnier <at> iro.umontreal.ca
Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Sun, 02 Feb 2025 17:02:04 +0000
"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Sun, 02 Feb 2025 12:50:15 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: eggert <at> cs.ucla.edu, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com, Andrea Corallo <acorallo <at> gnu.org>, Stefan Monnier <monnier <at> iro.umontreal.ca>
>>
>> Looking at bidi.c, I find bidi_get_type, which is essentially a single
>> switch statement, much easier to read than the new long switch statement
>> in in bidi_find_bracket_pairs:
>>
>> 	      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;
>> 		}
>
> This is an example of code where some value of an enumeration need
> special handling, and the rest don't need any handling at all.  IMO,
> we should ask ourselves what are we trying to accomplish by spelling
> out all of the "other" values instead of just 'default'?  If we only
> care for a handful of values, what kind of errors could slip us if we
> use 'default'?

As someone who's not very familiar with the bidi code, I would feel most
comfortable if this switch, with all cases, were in an inline function
which decides whether we set l2r_seen, r2l_seen, or neither, in this
context.  Ideally, in my case, with further comments, though trying to
understand this code without studying the Unicode Bidirectional
Algorithm will always be foolish...

However, I think bidi.c is the file I most strongly feel would not
benefit from -Wswitch-enum.

>> I think that could be rewritten to be more readable in a number of ways,
>> including, of course, putting a #pragma in.
>
> IMO, the way it should be rewritten (or not) depends on the answer to
> the above question.

Agreed.

Pip





This bug report was last modified 127 days ago.

Previous Next


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