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

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 75964 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>
Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Sat, 01 Feb 2025 18:54:40 +0000

"Paul Eggert" <eggert <at> cs.ucla.edu> writes:

> On 2025-02-01 01:02, Pip Cet wrote:
>> "Paul Eggert" <eggert <at> cs.ucla.edu> writes:
>>> Suppose the inference is wrong: how does one easily pacify the modified
>>> GCC? Is it something like "case +PVEC_NORMAL:"?
>>
>> I haven't submitted a patch yet, because I wasn't sure of such details.
>> In this case, yes, case +PVEC_NORMAL would work, but I had to test this :-)
>
> It'd be helpful to document this workaround in any proposed GCC patch
> (which I assume would include a doc patch).

Thanks for the subtle hint to actually document this.  Will do :-)

>>> Perhaps we should define a new macro eunreachable () that expands to
>>> eassume (false). eunreachable : unreachable :: eassert : assert. This
>>> might be a good idea regardless of anything else we do.
>>
>> I was thinking the same thing!
>>
>> I was going to propose emacs_unreachable (), but using the
>>
>> emacs_abort : emacs_unreachable :: eassert : eassume
>>
>> diagram.

> Either name would work of course. I mildly prefer just the "e" prefix.

Not so much about the name as about what it does by default.  We've
disagreed before on the merits of having eassume and eassert: I still
think we should have just one, and GCC can figure out itself whether the
information is useful for optimization (treat it as eassume) or not
(treat it as eassert)!

I don't know which builds eunreachable () would actually be treated as
unreachable in.  All optimized builds?  Only builds that also omit
emacs_abort ()?  What about machines where there's a single byte
"abort" instruction?

>>>   Perhaps the new GCC behavior should be enabled
>>> by a new GCC option (not -Wswitch-enum), so that it's easy to test
>>> whether the proposed GCC behavior is available.
>>
>> I believe that applies to the "mismatched enum" code (two different
>> enums/enums and ints), but simply inferring that a switch is an enum
>> switch because all of its case labels are seems safe to me.  But I'll
>> think about it some more.
>
> As long as there's an Autoconfy way to tell whether the new GCC behavior
> is present we should be OK. This could be via a test program compiled
> with the relev antGCC options. If that approach works, it might be
> better to fold the new behavior into -Wswitch-enum.

I don't think people will be very happy if we don't even try
understanding chained enums.  They're quite common.  And while detecting
enums with at least two distinct values forming a range is easy to do in
theory, it might use significant CPU resources on some builds.

> As you say, this requires some thought.

I'll work on the "infer type from case label" thing first.  That's
small, manageable, useful, and if I can't even get that in what's the
point writing long code to determine whether an enum is switchable?

>>>> +ALL_CFLAGS = ${BASE_CFLAGS} ${PROFILING_CFLAGS} ${LDFLAGS} ${CPPFLAGS} ${CFLAGS} ${PACIFYING_CFLAGS}
>>>>   CPP_CFLAGS = ${BASE_CFLAGS} ${PROFILING_CFLAGS} ${CPPFLAGS} ${CFLAGS}
>>>>   o
>>>>   ALL_CXXFLAGS = $(filter-out ${NON_CXX_CFLAGS},${BASE_CFLAGS}) \
>>>>     ${PROFILING_CFLAGS} ${LDFLAGS} ${CPPFLAGS} ${CFLAGS} ${CXXFLAGS} ${HAIKU_CFLAGS}
>>>>
>>>> +unfixed_Wswitch_obj += ctags${EXEEXT}
>>>> +unfixed_Wswitch_obj += etags${EXEEXT}
>>>
>>> This sort of thing can be confusing. Do we need it at all? That is, once
>>> the fixes are in the C source files we shouldn't need to change the
>>> Makefile.
>>
>> That was my initial reason for making it so unfixed_Wswitch_obj would
>> eventually be empty.
>
> It's the usual tradeoff between keeping each patch small and simple, vs
> minimizing the total size of all the patches. Here it might be better to
> opt for the latter, as changes like the above are a turnoff.

I'm not sure whether you're suggesting to list only the remaining
"unfixed" (yes, yes, better name) files somewhere, or something else
that I don't understand :-)

>> I've been bitten by "old" #pragmas disabling "bogus" warnings which
>> turned out not to be bogus before, and I think the #pragma syntax is
>> horrible, so I'd prefer the case +A: approach.
>
> I also dislike the pragmas, which is why I like to adorn them with
> comments containing the relevant GCC bug numbers. The intent is that the
> pragmas are "temporary" and should go away once the GCC bugs are fixed.

Thank you so much for the bug numbers! I'd go further and put the
pragmas into single-purpose include files, where we absolutely need
them.

_Pragma, well.

>> The modified GCC is in no way necessary to switch to switch-enum; it
>> does catch some more cases, but those cases caught by plain old GCC are
>> those we should fix first.
>
> Sounds good.

Of course it'd be even nicer if it were to remind us to turn "unsigned bf
: 3" into an ENUM_BF, but I have no idea, er, leave it as an exercise
for the reader.

I'll continue working on this.

Pip

Previous Next

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