Date: Thu, 27 Sep 2012 15:49:56 +0200 From: Dimitry Andric <dimitry@andric.com> To: mexas@bristol.ac.uk Cc: freebsd-ports@freebsd.org Subject: Re: clang dangling else help Message-ID: <50645984.3070208@andric.com> In-Reply-To: <201209271216.q8RCGdWU001139@mech-cluster241.men.bris.ac.uk> References: <201209271216.q8RCGdWU001139@mech-cluster241.men.bris.ac.uk>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
^
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?50645984.3070208>