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

From: Pip Cet <[email-masked]>
Date: Sat, 01 Feb 2025 18:54:40 +0000

"Paul Eggert" <[email-masked]> writes:

> On 2025-02-01 01:02, Pip Cet wrote:
>> "Paul Eggert" <[email-masked]> 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