GNU bug report logs - #23771
Eliminating compiler warnings

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 23771 in the body.
You can then email your comments to 23771 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Ken Brown <kbrown <at> cornell.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: Eliminating compiler warnings
Date: Tue, 14 Jun 2016 22:04:28 -0400

[Message part 1 (text/plain, inline)]

I would like to get rid of all compiler warnings in the Cygwin builds 
(X11, w32, and nox).  There are currently none in the X11 build, but 
there are a lot in the other builds.  Many of the warnings in the w32 
build probably occur in the MinGW build also.  And all of the warnings 
in the nox build would occur in any build without a window system.

The seven patches attached attempt to eliminate all the currently 
existing warnings.  There is one patch for each type of warning.  Since 
they affect other platforms besides Cygwin, I won't install them until 
someone has had a chance to review them.  There's obviously no rush 
about this.

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.

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.

Ken

[0001-Eliminate-noreturn-warnings-if-there-s-no-window-sys.patch (text/plain, attachment)]

[0002-Fix-unused-variable-compiler-warnings.patch (text/plain, attachment)]

[0003-Avoid-empty-body-compiler-warnings.patch (text/plain, attachment)]

[0004-Fix-format-compiler-warnings.patch (text/plain, attachment)]

[0005-Don-t-define-unneeded-function-on-Cygwin.patch (text/plain, attachment)]

[0006-Avoid-jump-misses-init-compiler-warnings.patch (text/plain, attachment)]

[0007-Avoid-address-compiler-warnings.patch (text/plain, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 15 Jun 2016 17:44:10 +0300

> From: Ken Brown <kbrown <at> cornell.edu>
> Date: Tue, 14 Jun 2016 22:04:28 -0400
> 
> I would like to get rid of all compiler warnings in the Cygwin builds (X11, w32, and nox). There are currently none in the X11 build, but there are a lot in the other builds. Many of the warnings in the w32 build probably occur in the MinGW build also. And all of the warnings in the nox build would occur in any build without a window system.
> 
> The seven patches attached attempt to eliminate all the currently existing warnings. There is one patch for each type of warning. Since they affect other platforms besides Cygwin, I won't install them until someone has had a chance to review them. There's obviously no rush about this.

Thanks.  Some comments below.

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.

> 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.

> 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.

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/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.

> --- 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.  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.

> @@ -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.

> --- 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.

> +#ifdef HAVE_WINDOW_SYSTEM
>  	  struct glyph *g;
> -
> +#endif

Better moved inside the #ifdef where it's used, no?  (And please keep
the empty line between declarations and code.)

> +#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.  So any warnings you have here need to
be solved in some other way.  I would hate to re-introduce HAVE_MOUSE,
having worked so hard in the past on removing it, so perhaps show the
warnings you get, and let's take it from there.

> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -5195,10 +5195,12 @@ realize_basic_faces (struct frame *f)
>  static bool
>  realize_default_face (struct frame *f)
>  {
> +#ifdef HAVE_X_WINDOWS
>    struct face_cache *c = FRAME_FACE_CACHE (f);
> +  struct face *face;
> +#endif
>    Lisp_Object lface;
>    Lisp_Object attrs[LFACE_VECTOR_SIZE];
> -  struct face *face;
> 
>    /* If the `default' face is not yet known, create it.  */
>    lface = lface_from_face_name (f, Qdefault, false);
> @@ -5288,10 +5290,10 @@ realize_default_face (struct frame *f)
>    eassert (lface_fully_specified_p (XVECTOR (lface)->contents));
>    check_lface (lface);
>    memcpy (attrs, XVECTOR (lface)->contents, sizeof attrs);
> -  face = realize_face (c, attrs, DEFAULT_FACE_ID);
> 
>  #ifdef HAVE_WINDOW_SYSTEM
>  #ifdef HAVE_X_WINDOWS
> +  face = realize_face (c, attrs, DEFAULT_FACE_ID);
>    if (FRAME_X_P (f) && face->font != FRAME_FONT (f))
>      {

This also looks wrong: the call to realize_face is NOT X-specific, see
the body of that function.  The value returned by realize_face is
indeed used only on X, but the real effect of the call is by side
effect: it adds the new face to the frame's face cache.

One possible way of getting rid of the warning is something like this:

 #ifdef HAVE_X_WINDOWS
 face = realize_face (c, attrs, DEFAULT_FACE_ID);
 #else
 (void) realize_face (c, attrs, DEFAULT_FACE_ID);
 #endif

> --- 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?

The rest looks OK to me, thanks (but I didn't try to compile with
them).

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

From: Richard Stallman <rms <at> gnu.org>
To: Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 15 Jun 2016 16:24:51 -0400

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > I would like to get rid of all compiler warnings in the Cygwin builds 
  > (X11, w32, and nox).  There are currently none in the X11 build, but 
  > there are a lot in the other builds.

It could be that the best way to get rid of some of these warnings
is to delete some -W option that causes GCC to issue them.

In the case of _Noreturn, isn't there a simpler approach,
defining a macro that expands into either _Noreturn or nothing?
Or defining _Noreturn as a no-op when there is no other definition?

  > +#ifdef HAVE_WINDOW_SYSTEM
  >        int fringe_bitmap;
  > +#endif

Surely it is easier to put in some code that will
unconditionally use that variable.  But maybe just delete
the -W option that causes these warnings.

To clutter up the code to placate a finicky compiler is a change
for the worse.  If the compiler is so finicky only because a special
option asked it to be, why be massochistic?


-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

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

From: Ken Brown <kbrown <at> cornell.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 23771 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 15 Jun 2016 21:38:19 -0400

On 6/15/2016 10:44 AM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown <at> 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

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

From: Ken Brown <kbrown <at> cornell.edu>
To: rms <at> gnu.org
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 15 Jun 2016 21:41:58 -0400

On 6/15/2016 4:24 PM, Richard Stallman wrote:
> To clutter up the code to placate a finicky compiler is a change
> for the worse.  If the compiler is so finicky only because a special
> option asked it to be, why be massochistic?

I'm just using the warning options that are enabled by default for a 
build from a git checkout of the master branch.  In general, I think the 
warnings are useful.  But I agree that I went too far in trying to get 
rid of them all.  I'm now thinking that I should just live with the ones 
that can't be fixed without cluttering the code.  See my reply to Eli.

Ken

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org, eggert <at> cs.ucla.edu
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Thu, 16 Jun 2016 18:14:37 +0300

> Cc: 23771 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
> From: Ken Brown <kbrown <at> cornell.edu>
> Date: Wed, 15 Jun 2016 21:38:19 -0400
> 
> > 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)
>           ^

For this one, I'd suggest to make unicode_append_menu a variable in
the Cygwin build as well, and then do this:

  AppendMenuW_Proc unicode_append_menu = AppendMenuW;

> >> +#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)
>   ^

How about making check_window_system compile in the no-X builds?

> >> --- 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)));
>                              ^

One way of fixing this is to add those braces.

> But I'd be fine with just disabling this warning, at least for the w32 
> builds.

Right.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>, Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Thu, 16 Jun 2016 08:50:16 -0700

Eli Zaretskii wrote:
> One way of fixing this is to add those braces.

The accepted way to fix this is to use a do-while. See, e.g., the definition of 
DEFVAR_LISP in lisp.h.

The warning is helpful to catch stray semicolons, e.g., 'if (n == 0); ...'.

Message #28 received at 23771-done <at> debbugs.gnu.org (full text, mbox):

From: Ken Brown <kbrown <at> cornell.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: eggert <at> cs.ucla.edu, 23771-done <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Mon, 20 Jun 2016 23:11:03 -0400

Thanks for all the suggestions and corrections.  I've fixed everything 
(I hope) and pushed to master.

Closing.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 22 Jun 2016 03:12:59 +0200

[Message part 1 (text/plain, inline)]

Thanks for all the work in improving static checking for Cygwin builds. 
I just now checked the attached patch into master, which I hope improves 
on it. I tested it on Fedora (both with and without window systems).

I had one problem with the recent changes, in that they suppressed all 
warnings about jumps over AUTO_STRING calls. That's pretty drastic, as 
the warnings are typically useful, so the attached patch reverts that. 
Can you let me know which call needs the warning suppressed in the 
Cygwin-specific code in the new master? I can suggest something which 
disables the warning just for that call.

[0001-Improve-without-x-GCC-pacification.patch (text/x-patch, attachment)]

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

From: martin rudalics <rudalics <at> gmx.at>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 22 Jun 2016 08:10:35 +0200

> +/* Get a frame's window system dimension.  If no window system, this is 0.  */
>
> +INLINE int
> +frame_dimension (int x)

frame_dimension sounds misleading to me.  Couldn't you call it something
like frame_area_dimension or frame_subarea_dimension instead?

Thanks, martin

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

From: Andy Moreton <andrewjmoreton <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 22 Jun 2016 15:04:39 +0100

On Wed 22 Jun 2016, Paul Eggert wrote:

> Thanks for all the work in improving static checking for Cygwin builds. I just
> now checked the attached patch into master, which I hope improves on it. I
> tested it on Fedora (both with and without window systems).
>
> I had one problem with the recent changes, in that they suppressed all
> warnings about jumps over AUTO_STRING calls. That's pretty drastic, as the
> warnings are typically useful, so the attached patch reverts that. Can you let
> me know which call needs the warning suppressed in the Cygwin-specific code in
> the new master? I can suggest something which disables the warning just for
> that call.

Your patch changed FRAME_INTERNAL_BORDER_WIDTH() from a macro to an
inline function, which breaks the mingw64 build:

../../src/w32fns.c: In function 'x_set_internal_border_width':
../../src/w32fns.c:1661:39: error: lvalue required as left operand of assignment
       FRAME_INTERNAL_BORDER_WIDTH (f) = border;
                                       ^
With FRAME_INTERNAL_BORDER_WIDTH() as a macro, it builds successfully.

    AndyM

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

From: Ken Brown <kbrown <at> cornell.edu>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 22 Jun 2016 10:06:23 -0400

On 6/21/2016 9:12 PM, Paul Eggert wrote:
> Thanks for all the work in improving static checking for Cygwin builds.
> I just now checked the attached patch into master, which I hope improves
> on it. I tested it on Fedora (both with and without window systems).

Thanks.  That's a big improvement.

> I had one problem with the recent changes, in that they suppressed all
> warnings about jumps over AUTO_STRING calls. That's pretty drastic, as
> the warnings are typically useful, so the attached patch reverts that.
> Can you let me know which call needs the warning suppressed in the
> Cygwin-specific code in the new master? I can suggest something which
> disables the warning just for that call.

It's in line 7057 of w32fns.c.  I see that you fixed a similar warning 
in xfns.c in commit e0400b7 by moving the AUTO_STRING call.  I've just 
done the same thing in commit bbc58fe.

Ken

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

From: Ken Brown <kbrown <at> cornell.edu>
To: Andy Moreton <andrewjmoreton <at> gmail.com>, 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Wed, 22 Jun 2016 10:10:48 -0400

On 6/22/2016 10:04 AM, Andy Moreton wrote:
> Your patch changed FRAME_INTERNAL_BORDER_WIDTH() from a macro to an
> inline function, which breaks the mingw64 build:
>
> ../../src/w32fns.c: In function 'x_set_internal_border_width':
> ../../src/w32fns.c:1661:39: error: lvalue required as left operand of assignment
>        FRAME_INTERNAL_BORDER_WIDTH (f) = border;

I fixed this in commit 81fc9a7.

Ken

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: martin rudalics <rudalics <at> gmx.at>, Ken Brown <kbrown <at> cornell.edu>
Cc: 23771 <at> debbugs.gnu.org
Subject: Re: bug#23771: Eliminating compiler warnings
Date: Thu, 23 Jun 2016 09:15:48 +0200

On 06/22/2016 08:10 AM, martin rudalics wrote:
> frame_dimension sounds misleading to me.  Couldn't you call it something
> like frame_area_dimension or frame_subarea_dimension instead?
Sure, please feel free to change the name, I didn't think much about it.

Previous Next

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