From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#23771: Eliminating compiler warnings Date: Wed, 15 Jun 2016 17:44:10 +0300 Message-ID: <83bn32eclh.fsf@gnu.org> References: <08ec82d8-305e-5a95-7fbb-76162a49f5cc@cornell.edu> Reply-To: Eli Zaretskii NNTP-Posting-Host: plane.gmane.org X-Trace: ger.gmane.org 1466001870 22921 80.91.229.3 (15 Jun 2016 14:44:30 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 15 Jun 2016 14:44:30 +0000 (UTC) Cc: 23771@debbugs.gnu.org To: Ken Brown Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Wed Jun 15 16:44:19 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1bDC3D-0005R6-AL for geb-bug-gnu-emacs@m.gmane.org; Wed, 15 Jun 2016 16:44:15 +0200 Original-Received: from localhost ([::1]:42611 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDC3C-00017J-Ng for geb-bug-gnu-emacs@m.gmane.org; Wed, 15 Jun 2016 10:44:14 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55648) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDC34-00016J-W2 for bug-gnu-emacs@gnu.org; Wed, 15 Jun 2016 10:44:08 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDC30-0001Rv-Po for bug-gnu-emacs@gnu.org; Wed, 15 Jun 2016 10:44:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:58292) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDC30-0001Rr-LR for bug-gnu-emacs@gnu.org; Wed, 15 Jun 2016 10:44:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1bDC30-0005my-H3 for bug-gnu-emacs@gnu.org; Wed, 15 Jun 2016 10:44:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 15 Jun 2016 14:44:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 23771 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 23771-submit@debbugs.gnu.org id=B23771.146600180722204 (code B ref 23771); Wed, 15 Jun 2016 14:44:02 +0000 Original-Received: (at 23771) by debbugs.gnu.org; 15 Jun 2016 14:43:27 +0000 Original-Received: from localhost ([127.0.0.1]:42396 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bDC2R-0005m4-6B for submit@debbugs.gnu.org; Wed, 15 Jun 2016 10:43:27 -0400 Original-Received: from eggs.gnu.org ([208.118.235.92]:36169) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bDC2O-0005lq-PC for 23771@debbugs.gnu.org; Wed, 15 Jun 2016 10:43:26 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDC2F-0001Hd-Tx for 23771@debbugs.gnu.org; Wed, 15 Jun 2016 10:43:19 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:48379) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDC2F-0001HR-Q0; Wed, 15 Jun 2016 10:43:15 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:1605 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bDC2D-0001H8-AG; Wed, 15 Jun 2016 10:43:14 -0400 In-reply-to: <08ec82d8-305e-5a95-7fbb-76162a49f5cc@cornell.edu> (message from Ken Brown on Tue, 14 Jun 2016 22:04:28 -0400) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:119580 Archived-At: > From: Ken Brown > 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).