Package: emacs;
Reported by: Pip Cet <pipcet <at> protonmail.com>
Date: Fri, 31 Jan 2025 09:41:02 UTC
Severity: wishlist
Message #227 received at 75964 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Paul Eggert <eggert <at> cs.ucla.edu> Cc: pipcet <at> protonmail.com, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com Subject: Re: bug#75964: Switching the Emacs build to -Wswitch-enum in src/ Date: Thu, 06 Feb 2025 09:41:03 +0200
> Date: Wed, 5 Feb 2025 11:20:47 -0800
> Cc: pipcet <at> protonmail.com, 75964 <at> debbugs.gnu.org, stefankangas <at> gmail.com
> From: Paul Eggert <eggert <at> cs.ucla.edu>
>
> > 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.
My point is that it is not easy, to say the least, "to do it right"
when the code was written by someone else, especially if it is complex
(etags.c is a good case in point). This is what we are discussing, or
at least this is a part of our discussion; if you are talking about
guidelines for future code writers, I will probably agree, but will
point out that there's a non-vanishing probability that quite a few
contributors will not follow such guidelines, or maybe are even unable
to.
> > 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.
Where it's easy, and the code's intent is clear and relatively simple,
I agree. But we should avoid blindly rewriting each and every switch
and enum along these lines, especially where the code is NOT simple
and clear, because there the risk of introducing problems and making
the code harder to read and understand outweighs the small gains from
these changes.
> > 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.
Yes, but such casts are easy to detect when reviewing code, and we
should reject such type-casts in our code, except in rare places where
they are a must (imposed by restrictions of external APIs and data
types).
> That would introduce more possibilities for mistakes than +E does.
I doubt that. The advantage is that the code is much less surprising.
> When possible (as is the case here) we should avoid using casts.
I agree, but I think this is one of the cases where we could use it.
I would even agree to using
switch (E + 0)
although I suspect that some GCC warning will flag it, and if it
doesn't today, it will probably do so in some non-t-distant future.
But even that is easier on the code reader that +E.
> >> 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.
My point is that this coding trick, or anything close to it, is never
mentioned explicitly, neither in the C Standard, nor in several other
documents on C that I looked in. E.g., in all of Gnulib it is used
exactly 3 times. So it should be quite clear that this technique is
not widely used, and thus is relatively unknown to people. And it
looks strange enough to raise brows. Which, from where I stand, is a
disadvantage.
> >> 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'.
I'm not saying that the C Standard doesn't cover this, or that it's
invalid C. I'm saying that a naïve reading of that text will never
explain the semantics of +E, let alone how it's related to the issue
of switch statements on enums.
> >> 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.
Yes, I now know it, after asking many questions. We do not want to
use in our code techniques that require prolonged discussions to
understand them, at least not use them widely.
> 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))
"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. People should
have at least mild chances to understand the code without consulting
the definition of every macro it uses, so the names of the macros
should express what they do well enough to allow such code reading.
> > 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 we want to take into account the possibility of corrupted code,
then all bets are off, and this discussion is not useful. E.g., why
assume that uni-bidi is corrupted, but the code which checks validity
of bidi_type_t is not? uni-bidi is preloaded (via charprop.el) and is
either in the pdumper file or (if the build is with native
compilation) in a shared library loaded at startup, so it is basically
part of the Emacs binary. As long as we don't have protection from
corrupted Emacs binary, why should we seriously consider corruption in
preloaded Lisp files that are essential for basic Emacs operation?
> 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?
The comment there tries to explain that, but it is possible that it is
not detailed enough for someone who is not as "immersed" in this as I
am.
Each Unicode character has some bidi-class property, whose value is
never UNKNOWN_BT. The file DerivedBidiClass.txt in the Unicode
Character Database (UCD) says:
# Bidi Class (listing UnicodeData.txt, field 4: see UAX #44: https://www.unicode.org/reports/tr44/)
# Unlike other properties, unassigned code points in blocks
# reserved for right-to-left scripts are given either values R or AL,
# and unassigned code points in the Currency Symbols block are given the value ET.
# For details see the @missing lines below.
#
# The unassigned code points that default to BN have one of the following properties:
# Default_Ignorable_Code_Point
# Noncharacter_Code_Point
#
# For all other cases:
# All code points not explicitly listed for Bidi_Class
# have the value Left_To_Right (L).
Thus, the bidi-class code we get from bidi_type_table for any
character can never be UNKNOWN_BT (which is not a bidi type, just an
indication of "no bidi-class information"; the valid bidi types start
with STRONG_L). So 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
Moreover, all of the code in bidi.c is written under an implicit
assumption that each character has a bidi_type_t value that is _not_
UNKNOWN_BT; if that assumption breaks, I don't know what will happen,
but it's quite possible that Emacs will display garbage or crash or
cause the end of the world as we know it.
> 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.
OK, I will try to expand the comment. Is the explanation above
sufficient?
> > 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.
No, it was to bidi_get_type.
> 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.
Just 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.
> >> 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.
I don't think it is useful to consider the case of corrupted uni-bidi,
see above. But 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).
> >> - 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.
Then 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?
> 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.
I don't mind installing it, subject to the above considerations of
making a macro with a mnemonic name for that purpose, but I would like
first to understand, in each case, what does adding that gain us in
terms of better code readability and future extensions.
Thanks.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.