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

From: Eli Zaretskii <[email-masked]>
Date: Sun, 09 Feb 2025 09:29:41 +0200

> Date: Sat, 8 Feb 2025 19:40:21 -0800
> Cc: [email-masked], [email-masked], [email-masked]
> From: Paul Eggert <[email-masked]>
> 
> 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 

I have no objections to this patch, but it would be weird if bidi.c
were the only C source file to use this technique.

> > 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.

UNKNOWN_BT is zero, so it means some character has its bidi-class left
as nil (a.k.a. "uninitialized").  That's not corruption, that's a bug
in Emacs or in the Emacs build procedure.

> > 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?

Extremely unlikely, to say the least.  We don't actively protect
against arbitrary corruptions in our *.elc or *.eln files, are we?
That's the same class of problems as what you mention here.

> That is, one could get any value outside the range 
> STRONG_L..NEUTRAL_ON

Not if uniprop char-table code does its job correctly.

> > 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?

Depends on the meaning of "benefit", I suppose.  Currently, the calls
to bidi_get_type are where having an invalid type will have
catastrophic consequences.

> > 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 references to the UBA are supposed to be a better tool, since GCC
has no way of knowing where omitting an enum value is a mistake and
where it is intentional.

> 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.

I prefer comments which say that explicitly.  The promotion trick is
for the compiler, not for humans.  For that reason, I think using
INT_PROMOTE should be accompanied by an appropriate comment.

Thanks.