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

Previous Next

Full log

Message #38 received at 75964 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 75964 <at> debbugs.gnu.org
Subject: Re: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Fri, 31 Jan 2025 14:25:21 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Fri, 31 Jan 2025 12:48:02 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: 75964 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> 0. do nothing, keeping bidi.o on the list of unfixed Wswitch objects,
>> >> explaining that too many large switches are in use for the result of (1)
>> >> to be readable
>> >> 1. bite the bullet and apply a 184-line diff to bidi.c.
>> >> 2. add #pragma statements in bidi.c around the five large switch
>> >> statements, giving us the benefit of warnings for the other switch
>> >> statements in bidi.c
>> >> 3. turn bidi_type_t into a real integer type to avoid the warning
>> >> 4. cast bidi_type_t to a real integer type in the switch statement.
>> >> -1. use if() instead
>> >> -2. use arrays instead
>> >> -3. use arrays of function pointers instead
>> >
>> > That might be fine when the enumeration is ours, but if it comes from
>> > a system header file, the only practical option is 0, with 1 being a
>> > very distant second (having a switch with many dozens of values is
>> > hardly a good thing).
>>
>> You're talking about a different problem then.  If an
>> externally-controlled enum is expected to grow, we want a compiler
>> warning to let us know when this happens, but we also need a default:
>> handler that deals with this case.  So the right option is that we don't
>> need any of the extra options, the default behaviour will be just fine.
>
> Maybe.  I'm not yet sure.
>
> I also think we should maybe take a step back and summarize the
> problems the use of -Wswitch-enum everywhere is supposed to help us
> solve.  You mentioned other approaches that have limitations, but what
> are the specific problems in our sources that this and the other
> approaches aim to solve?

I'd like to say "look at these bugs we've fixed", but so far the list is
quite short: rsvg misbehaved for a newly-added unit, and treesitter
produced a suboptimal error message.

> I think we need to have the problems we want to solve in mind when
> considering the solutions, because the solutions are not without
> costs.

Agreed.  I'm still looking for a smoking gun: a case label which we
forgot and which caused the default case label to do the wrong thing.

The only reason I see to hurry here is that various people are
pushing changes to master which follow this or that solution, and
usually it's not the right one (in my very personal view).  If we could
agree to stop doing that and leave switch statements alone for now, I'd
be very happy (i.e. apply the three reverts to pdumper.c but not the
fourth patch in the last series).

> And if we end up leaving many source files in the "unfixed"
> class, then maybe we should turn this around and only use this switch
> for those few files where it doesn't cause us problems for which
> solutions we don't want to pay those costs.

Thanks!  That's a great idea!  I also shouldn't have used "unfixed" as a
name: there's nothing wrong with disabling warnings per file, and
different files have different requirements, and all that's perfectly
okay and not anyone's refusal to fix anything.

Thanks for the feedback!

Pip

Previous Next

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