From owner-freebsd-ports@FreeBSD.ORG Thu Sep 27 13:50:04 2012 Return-Path: Delivered-To: freebsd-ports@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C3301106564A for ; Thu, 27 Sep 2012 13:50:04 +0000 (UTC) (envelope-from dimitry@andric.com) Received: from tensor.andric.com (tensor.andric.com [87.251.56.140]) by mx1.freebsd.org (Postfix) with ESMTP id 09E9E8FC1A for ; Thu, 27 Sep 2012 13:50:03 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:98da:7fd6:da8b:ff5b] (unknown [IPv6:2001:7b8:3a7:0:98da:7fd6:da8b:ff5b]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 7CC325C59; Thu, 27 Sep 2012 15:49:56 +0200 (CEST) Message-ID: <50645984.3070208@andric.com> Date: Thu, 27 Sep 2012 15:49:56 +0200 From: Dimitry Andric User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20120905 Thunderbird/16.0 MIME-Version: 1.0 To: mexas@bristol.ac.uk References: <201209271216.q8RCGdWU001139@mech-cluster241.men.bris.ac.uk> In-Reply-To: <201209271216.q8RCGdWU001139@mech-cluster241.men.bris.ac.uk> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: freebsd-ports@freebsd.org Subject: Re: clang dangling else help X-BeenThere: freebsd-ports@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting software to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Sep 2012 13:50:04 -0000 On 2012-09-27 14:16, Anton Shterenlikht wrote: > Building my port with clang I get this warning > (gcc doesn't pick this up): > > x11.c:1543:5: warning: add explicit braces to avoid dangling else [-Wdangling-else] > else if (actual_type != None) > ^ > 1 warning generated. If the warning isn't fatal (e.g. you are not using -Werror), and you know the code is right, you could just ignore the warning. Or are you trying to make it warning-free? > I think I understand what's going on, > but wanted somebody to double check. > > This is the relevant fragment: > > 1511 static void freePrevious(dpy, w) > 1512 Display *dpy; > 1513 Window w; > 1514 { > 1515 Pixmap *pm; > 1516 Atom actual_type; > 1517 int format; > 1518 unsigned long nitems; > 1519 unsigned long bytes_after; > 1520 > 1521 /* intern the property name */ > 1522 Atom atom = XInternAtom(dpy, RETAIN_PROP_NAME, 0); > 1523 > 1524 /* look for existing resource allocation */ > 1525 if ((XGetWindowProperty(dpy, w, atom, 0, 1, 1 /*delete*/, > 1526 AnyPropertyType, &actual_type, > 1527 &format, &nitems, &bytes_after, > 1528 (unsigned char **) &pm) == Success) && > 1529 (nitems == 1)) > 1530 if ((actual_type == XA_PIXMAP) && (format == 32) && > 1531 (nitems == 1) && (bytes_after == 0)) > 1532 { > 1533 /* blast it away, but first provide new X error handler in case > 1534 * the client that installed the RETAIN_PROP_NAME (_XSETROOT_ID) > 1535 * property on the root window has already terminated > 1536 */ > 1537 orig_error_handler = XSetErrorHandler(xkill_handler); > 1538 XKillClient(dpy, (XID) *pm); > 1539 XSync(dpy, False); > 1540 XSetErrorHandler(orig_error_handler); > 1541 XFree((void *) pm); > 1542 } > 1543 else if (actual_type != None) > 1544 { > 1545 fprintf(stderr, > 1546 "%s: warning: invalid format encountered for property %s\n", > 1547 RETAIN_PROP_NAME, progname); > 1548 } > 1549 } > > I think, to preserve the logic, this should be changed to: > > 1511 static void freePrevious(dpy, w) > 1512 Display *dpy; > 1513 Window w; > 1514 { > 1515 Pixmap *pm; > 1516 Atom actual_type; > 1517 int format; > 1518 unsigned long nitems; > 1519 unsigned long bytes_after; > 1520 > 1521 /* intern the property name */ > 1522 Atom atom = XInternAtom(dpy, RETAIN_PROP_NAME, 0); > 1523 > 1524 /* look for existing resource allocation */ > 1525 if ((XGetWindowProperty(dpy, w, atom, 0, 1, 1 /*delete*/, > 1526 AnyPropertyType, &actual_type, > 1527 &format, &nitems, &bytes_after, > 1528 (unsigned char **) &pm) == Success) && > 1529 (nitems == 1)) > { > 1530 if ((actual_type == XA_PIXMAP) && (format == 32) && > 1531 (nitems == 1) && (bytes_after == 0)) > 1532 { > 1533 /* blast it away, but first provide new X error handler in case > 1534 * the client that installed the RETAIN_PROP_NAME (_XSETROOT_ID) > 1535 * property on the root window has already terminated > 1536 */ > 1537 orig_error_handler = XSetErrorHandler(xkill_handler); > 1538 XKillClient(dpy, (XID) *pm); > 1539 XSync(dpy, False); > 1540 XSetErrorHandler(orig_error_handler); > 1541 XFree((void *) pm); > 1542 } > 1543 else if (actual_type != None) > 1544 { > 1545 fprintf(stderr, > 1546 "%s: warning: invalid format encountered for property %s\n", > 1547 RETAIN_PROP_NAME, progname); > 1548 } > } > 1549 } Yes, this is precisely what clang means, though the diagnostic can be a bit confusing. It tells you the else in line 1543 is ambiguous, since some people might be tempted to believe it belongs to the first if(), instead of the second one. In fact, even my auto-indenting editor also got it wrong at first. :) Simplified, this example becomes: void foo(int i, int j, int k) { if (i) if (j) { bar(); } else if (k) { baz(); } } So clang is suggesting to change that to: void foo(int i, int j, int k) { if (i) { if (j) { bar(); } else if (k) { baz(); } } } In my opinion the diagnostic could be more helpful though, and point you at the if() statements that needed additional braces. In this case, gcc 4.8 is indeed more helpful: dangling.c: In function 'foo': dangling.c:6:6: warning: suggest explicit braces to avoid ambiguous 'else' [-Wparentheses] if (i) ^