Re: Preview: portable dumper

[Daniel Colascione]

On Feb 16, 2018 1:49 PM, Paul Eggert <address@hidden> wrote:

On 02/16/2018 01:23 PM, Daniel Colascione wrote:
> Outside of certain specialized casting facilities
DEFINE_TOLISP_FUNC is exactly the sort of specialized casting facility
where -Wconversion is more likely to mess up than usual, which is why I
suggested disabling -Wconversion there in pdumper.c. -Wconversion is not
as useful elsewhere, which is why I suggested leaving INTEGER_TO_CONS alone.

> , -Wconversion ought to warn only on things that are actually
> problems. Have a counter-example

Sure, revert the pdumper change to INTEGER_TO_CONS but leave pdumper.c
alone, by applying the attached patch to pdumper. The compile on a
platform where EMACS_INT is 32 bits, using GCC 7.3.1 20180130 (Red Hat
7.3.1-2). The resulting diagnostic is a false alarm:

pdumper.c: In function ‘intmax_t_to_lisp’:
pdumper.c:694:29: error: conversion to ‘EMACS_INT {aka int}’ from
‘intmax_t {aka long long int}’ may alter its value [-Werror=conversion]
     return INTEGER_TO_CONS (value);  \

I did give myself an out by talking about specialized constructs :-) I think that outside of a few places like this, the code mostly won't need much special attention to compile cleanly under Wconversion.

                             ^
lisp.h:3572:19: note: in definition of macro ‘INTEGER_TO_CONS’
    ? make_number (i)          \
                   ^
pdumper.c:698:1: note: in expansion of macro ‘DEFINE_TOLISP_FUNC’
 DEFINE_TOLISP_FUNC (intmax_t_to_lisp, intmax_t);
 ^~~~~~~~~~~~~~~~~~

This false alarm is due to a GCC bug. GCC is supposed to take
expressions like this one (adapted from INTEGER_TO_CONS):

  (MOST_NEGATIVE_FIXNUM <= i && i <= MOST_POSITIVE_FIXNUM
   ? make_number (i)
   : something_else (i))

and evaluate make_number (i) in the context of i being in range for
fixnums (which means i is in range for EMACS_INT). In this particular
case, GCC has a bug where it forgets i's range, and thus the false alarm.

I'd file a bug report with the GCC folks, but in the past my experience
is that they don't take -Wconversion bug reports all that seriously. In
practice, -Wconversion isn't good enough for high-quality code like what
Emacs should be.

I'm not sure what you mean by high quality in this context.

> -Wconversion helped me find real bugs and otherwise mysterious bugs in
> pdumper.
Yes, -Wconversion can find bugs, particularly the first time one writes
a program when the program has lots of bugs that need finding. But its
false-alarm rate is too high to be useful in high-quality code. Although
it's OK to use -Wconversion temporarily in new or otherwise-buggy areas,
we shouldn't encourage its use in the part of Emacs that is intended to
be stable and high-quality. 

And if programmers were angels, no warnings would be necessary. The point is to *keep* the code high quality, and a warning for possibly unintended conversion helps  avoid bugs that are otherwise very difficult to  find. There are definitely trade-offs, but I still think the syntactic noise of Wconversion can mostly be hidden.