bug#23771: Eliminating compiler warnings

[Ken Brown]

On 6/15/2016 10:44 AM, Eli Zaretskii wrote:
> > From: Ken Brown <kbrown@cornell.edu>
> Btw, it would have helped if you'd show examples of warnings that
> eliminated by each patch in the set; for some of them, it's easy to
> guess, others not so easy.  I hope I did right.

Thanks for the review.  Sorry, I should have thought of giving examples 
of the warnings.

> > Two comments: First, patch 0006-... is there because there might be a jump over 
> > an AUTO_STRING call. (This happens exactly once in the Cygwin-w32 build.) It 
> > seems stupid to have to worry about this. An alternative would be to just 
> > disable the -Wjump-misses-init warning.
>
> The warning is IMO important, it's just too bad GCC has false
> positives with it.

OK.

> > Second, patch 0007-... is there because I couldn't think of a reasonable way to 
> > avoid -Waddress warnings when compiling w32fns.c, w32menu.c, and menu.c in the 
> > Cygwin-w32 build. Everything I thought of would have made the code very ugly. 
> > So I simply disabled that warning for the Cygwin-w32 build, and I took the 
> > liberty of doing the same thing for the MinGW build, which I think is also 
> > affected in some cases. If someone sees a better way of eliminating those 
> > warnings, that would be preferable.
>
> What warnings does that option produce?  I'm not sure I've seen any
> warnings about addresses, but maybe I misunderstand the nature of the
> warning.

Here's a typical example:

../../master/src/menu.c: In function ‘digest_single_submenu’:
../../master/src/menu.c:46:30: warning: the address of ‘AppendMenuW’ 
will always evaluate as ‘true’ [-Waddress]
 # define unicode_append_menu AppendMenuW
                              ^
../../master/src/menu.c:691:9: note: in expansion of macro 
‘unicode_append_menu’
     if (unicode_append_menu)
         ^
../../master/src/menu.c:46:30: warning: the address of ‘AppendMenuW’ 
will always evaluate as ‘true’ [-Waddress]
 # define unicode_append_menu AppendMenuW
                              ^
../../master/src/menu.c:767:9: note: in expansion of macro 
‘unicode_append_menu’
     if (unicode_append_menu)
         ^

> What follows is specific comments to some hunks.
>
> > +#else  /* not HAVE_WINDOW_SYSTEM */
> > +
> > +_Noreturn void
> > +decode_window_system_frame (Lisp_Object frame)
> > +{
> > +  error ("Window system is not in use");
> > +}
> > +
> > +_Noreturn void
> > +check_window_system (struct frame *f)
> > +{
> > +  error ("Window system is not in use");
> > +}
> > +
> > +#endif /* not HAVE_WINDOW_SYSTEM */
>
> What kind of warnings do you get without these changes?  I don't
> understand why this is needed.

A build with no windows system yields these warnings:

../../warnings/src/frame.c: In function ‘decode_window_system_frame’:
../../warnings/src/frame.c:119:1: warning: function might be candidate 
for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
 decode_window_system_frame (Lisp_Object frame)
 ^
../../warnings/src/frame.c: In function ‘check_window_system’:
../../warnings/src/frame.c:129:1: warning: function might be candidate 
for attribute ‘noreturn’ [-Wsuggest-attribute=noreturn]
 check_window_system (struct frame *f)
 ^

> > --- a/src/font.c
> > +++ b/src/font.c
> > @@ -2863,7 +2863,10 @@ font_open_entity (struct frame *f, Lisp_Object entity, 
> > int pixel_size)
> >    struct font_driver_list *driver_list;
> >    Lisp_Object objlist, size, val, font_object;
> >    struct font *font;
> > -  int min_width, height, psize;
> > +  int height, psize;
> > +#ifdef HAVE_WINDOW_SYSTEM
> > +  int min_width;
> > +#endif
> >
> >    eassert (FONT_ENTITY_P (entity));
> >    size = AREF (entity, FONT_SIZE_INDEX);
> > @@ -2907,10 +2910,12 @@ font_open_entity (struct frame *f, Lisp_Object entity, 
> > int pixel_size)
> >         Fcons (font_object, AREF (entity, FONT_OBJLIST_INDEX)));
> >
> >    font = XFONT_OBJECT (font_object);
> > +#ifdef HAVE_WINDOW_SYSTEM
> >    min_width = (font->min_width ? font->min_width
> >                : font->average_width ? font->average_width
> >                : font->space_width ? font->space_width
> >                : 1);
> > +#endif
>
> Why not move the declaration of min_width and the calculation of its
> value inside the #ifdef that uses it?  Then you'd have eliminated 2
> #ifdef's.

Good idea.  I'll do that.

> > --- a/src/frame.c
> > +++ b/src/frame.c
> > @@ -2143,13 +2143,12 @@ DEFUN ("iconify-frame", Ficonify_frame, Siconify_frame,
> >  If omitted, FRAME defaults to the currently selected frame.  */)
> >    (Lisp_Object frame)
> >  {
> > -  struct frame *f = decode_live_frame (frame);
> > -
> >    /* Don't allow minibuf_window to remain on an iconified frame.  */
> >    check_minibuf_window (frame, EQ (minibuf_window, selected_window));
> >
> >    /* I think this should be done with a hook.  */
> >  #ifdef HAVE_WINDOW_SYSTEM
> > +  struct frame *f = decode_live_frame (frame);
> >    if (FRAME_WINDOW_P (f))
> >        x_iconify_frame (f);
> >  #endif
>
> I understand the motivation, but this change has a downside: it
> changes the order of validity check of the function arguments, and it
> completely removes the check of the FRAME argument in the builds
> without X.  So I don't think we should make this change this way.

Yes, I was very careless about that.  I'm glad you caught it.

> We could do something like this instead:
>
>  #ifdef HAVE_WINDOW_SYSTEM
>  struct frame *f = decode_live_frame (frame);
>  #else
>  (void) decode_live_frame (frame);
>  #endif
>
> Or we could work around the warning in some other way, or even live
> with it.

I'm now thinking, especially in view of Richard's comments, that maybe 
we should just live with the "unused variable" warnings that can't be 
avoided without cluttering the code.

> > @@ -3015,13 +3014,12 @@ or bottom edge of the outer frame of FRAME relative to 
> > the right or
> >  bottom edge of FRAME's display.  */)
> >    (Lisp_Object frame, Lisp_Object x, Lisp_Object y)
> >  {
> > -  register struct frame *f = decode_live_frame (frame);
> > -
> >    CHECK_TYPE_RANGED_INTEGER (int, x);
> >    CHECK_TYPE_RANGED_INTEGER (int, y);
>
> >    /* I think this should be done with a hook.  */
> >  #ifdef HAVE_WINDOW_SYSTEM
> > +  register struct frame *f = decode_live_frame (frame);
> >    if (FRAME_WINDOW_P (f))
> >      x_set_offset (f, XINT (x), XINT (y), 1);
> >  #endif
>
> Same comment here: the order of checking the arguments is important to
> keep on all frames in all builds.

Yes, more carelessness on my part.

> > --- a/src/w32fns.c
> > +++ b/src/w32fns.c
> > @@ -6888,7 +6888,7 @@ A tooltip's maximum size is specified by 
> > `x-max-tooltip-size'.
> >  Text larger than the specified size is clipped.  */)
> >    (Lisp_Object string, Lisp_Object frame, Lisp_Object parms, Lisp_Object 
> > timeout, Lisp_Object dx, Lisp_Object dy)
> >  {
> > -  struct frame *tip_f;
> > +  struct frame *f, *tip_f;
>
> Please add comments explaining why 'f' is needed (here and
> elsewhere).  Someone who doesn't remember the w32 definition of
> FRAME_DISPLAY_INFO will stumble on this one.

OK

> > +#ifdef HAVE_WINDOW_SYSTEM
> >           struct glyph *g;
> > -
> > +#endif
>
> Better moved inside the #ifdef where it's used, no?

Yes.

> > +#ifdef HAVE_WINDOW_SYSTEM
> >           if (clear_mouse_face (hlinfo))
> >             cursor = No_Cursor;
> > -
> > +#endif
>
> This and other similar hunks that ifdef away clear_mouse_face are
> incorrect: Emacs supports mouse highlight on TTY frames (if the mouse
> is supported, e.g. via GPM or on MS-Windows/MS-DOS), so we must call
> clear_mouse_face there as well.

The same kind of carelessness yet again.  (I was just trying to get rid 
of the warning that 'cursor' is unused, but I obviously threw away too 
much.)  I'll fix all these and resend the patch.

> > --- a/src/conf_post.h
> > +++ b/src/conf_post.h
> > @@ -211,7 +211,7 @@ You lose; /* Emacs for DOS must be compiled with DJGPP */
> >  extern void _DebPrint (const char *fmt, ...);
> >  #  define DebPrint(stuff) _DebPrint stuff
> >  # else
> > -#  define DebPrint(stuff)
> > +#  define DebPrint(stuff) {}
> >  # endif
> >  #endif
>
> Yuck!  Can we simply not use the "empty body" warning option?  When is
> it important to flag an empty body of a function?

Here's a typical example:

Code like this:

  if (!f->output_data.w32->asked_for_visible)
    DebPrint (("frame %p (%s) reexposed by WM_PAINT\n", f,
               SDATA (f->name)));

leads to this warning (if EMACSDEBUG is not defined):

../../warnings/src/w32term.c: In function ‘w32_read_socket’:
../../warnings/src/w32term.c:4613:28: warning: suggest braces around 
empty body in an ‘if’ statement [-Wempty-body]
           SDATA (f->name)));
                            ^

But I'd be fine with just disabling this warning, at least for the w32 
builds.  Paul, do you think it's important to keep this warning?

Thanks again for the review.

Ken