GNU bug report logs -
#75964
Switching the Emacs build to -Wswitch-enum in src/
Previous Next
Full log
Message #236 received at 75964 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
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.
[0001-Pacify-gcc-Wswitch-enum-in-bidi.c.patch (text/x-patch, attachment)]
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.