Re: master a99ba59aa02: ; * src/pdumper.c (dump_do_fixup): Pacify GCC.

[Pip Cet]

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

> branch: master
> commit a99ba59aa02ef8cfd314737950b6cd8d97015925
> Author: Eli Zaretskii <eliz@gnu.org>
> Commit: Eli Zaretskii <eliz@gnu.org>
>
>     ; * src/pdumper.c (dump_do_fixup): Pacify GCC.

> ---
>  src/pdumper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/pdumper.c b/src/pdumper.c
> index 9f0447eb5aa..bfa790b963a 100644
> --- a/src/pdumper.c
> +++ b/src/pdumper.c
> @@ -3990,7 +3990,7 @@ dump_do_fixup (struct dump_context *ctx,
>    Lisp_Object arg = dump_pop (&fixup);
>    eassert (NILP (fixup));
>    dump_seek (ctx, dump_fixup_offset);
> -  intptr_t dump_value;
> +  intptr_t dump_value UNINIT;

Can we please stop rewriting every single switch statement again and
again, each time attempting to solve one problem which will make
another, worse one, pop up instead?

UNINIT, IMHO, is to be used in situations where GCC fails to discover
that a code path to a usage of the variable lacking an initializer is
impossible.

This is a very different case.  By removing the default case in the
switch statement, we made the code path which falls through the switch
statement without initializing dump_value reachable.  There's not even a
hint to the compiler that this is an unlikely code path, so the
resulting code will be significantly worse than what would have been
possible before.

I stand by my statement that there is currently no real good way to get
the right compiler warnings out of GCC in all cases.

Since there isn't such a way, we should not continue altering switch
statements in this or that way and add new "impossible" code paths for
GCC to optimize.  The warning is the symptom here: the problem is we've
added a new code path which will use uninitialized values but affects
the optimization of all other, intended, code paths.

Most importantly, we should not give up and use if () or, worse, look up
values in static arrays (the latter makes it very hard to change things
should it turn out that calculating the array value is difficult).  And,
I'm sorry, an array of function pointer that we switch on seems even
worse to me.

These changes do not improve Emacs.

Can we please just go back to the "default: emacs_abort();" we had
before?

if we're *really* sure that performance is more important than
correctness checking when ENABLE_CHECKING is not set (are we?  Were
those changes made mechanically or explained in the commit message?), we
can use default: eassume (false);

If we want extra warnings, and GCC cannot provide them otherwise, let's
use -Wswitch-enum and ignore the warning where GCC cannot do so
automatically.

"Static checking" is being thrown around a lot, and statically checking
that all enum values known at compile time are handled is a good thing,
but in many ABIs, it's valid to expand an enum without recompiling your
code.  This will mean that we lost a runtime abort and gained utterly
undefined behavior in such cases.  If we have to choose between dynamic
checking and static checking, the former will often work where the
latter wouldn't.  -Wswitch-enum and default: eassume (false); give us
both, though!

There are problems with -Wswitch-enum, but they're much less of a
problem than UNINIT in places where valid code paths are produced which
make use of the UNINIT value.

If we have decided to use UNINIT in such cases, we need to clearly
indicate this.

Pip