Strange difference between Clang and GCC's -Wparentheses

Currently Clang and GCC warn about:

(a || b && c)

Due to the very common mistake by programmers which ignores the relative operator precedence of || and &&. This forces the user to place explicit parentheses to group either (a || b) or (b && c) to be evaluated first.

However, GCC warns and Clang is silent about this:

(a || b && 1)

Worse, the same is true for:

(a || b && “foo”)

Now, I find Clang’s rationale seems somewhat reasonable: the grouping doesn’t change the truth table for this expression. However, I find this clever interpretation problematic for several reasons:

  1. It’s a complex rule to teach programmers with dubious gains. This exception doesn’t realistically make the warning’s false positives rare, it just eliminates one common (but not the only common) pattern. On the flip side, it is not a trivial rule to teach or predict for programmers. Any constant that we can evaluate to true will satisfy this.

  2. If the programmer expected the || to be evaluated first, they might well also expect the “a” expression to be evaluated first and short circuiting to take place. Even though the truth table is unimpacted, ‘assert(!ptr || ptr->empty() && “foo”);’ can segfault, which I think might surprise users.

  3. It makes it annoying to track warnings in both Clang and GCC. If you mostly develop with Clang, but someone else checks out your code with GCC it’ll suddenly start warning (much like it just did in a recent commit.)

Thoughts? I’m happy to fix this, the change is trivial in SemaExpr.cpp, but wanted to get some confirmation from Richard and Argyrios who have worked on this warning that such a change would be OK.

-Chandler

Currently Clang and GCC warn about:

(a || b && c)

Due to the very common mistake by programmers which ignores the relative operator precedence of || and &&. This forces the user to place explicit parentheses to group either (a || b) or (b && c) to be evaluated first.

However, GCC warns and Clang is silent about this:

(a || b && 1)

Worse, the same is true for:

(a || b && “foo”)

Now, I find Clang’s rationale seems somewhat reasonable: the grouping doesn’t change the truth table for this expression. However, I find this clever interpretation problematic for several reasons:

  1. It’s a complex rule to teach programmers with dubious gains. This exception doesn’t realistically make the warning’s false positives rare, it just eliminates one common (but not the only common) pattern. On the flip side, it is not a trivial rule to teach or predict for programmers. Any constant that we can evaluate to true will satisfy this.

  2. If the programmer expected the || to be evaluated first, they might well also expect the “a” expression to be evaluated first and short circuiting to take place. Even though the truth table is unimpacted, ‘assert(!ptr || ptr->empty() && “foo”);’ can segfault, which I think might surprise users.

Please ignore #2. Doh! ;]

For context, here’s Agyrios’s commit that implemented this functionality: http://llvm.org/viewvc/llvm-project?view=revision&revision=119533 - I personally don’t have email history going back that far to know if this was the result of some on-list review/discussion to provide more context to the decision.

My thoughts from the peanut gallery follow (but I don’t want them to be a loud voice since I think Apple probably has some relevant input here)

Currently Clang and GCC warn about:

(a || b && c)

Due to the very common mistake by programmers which ignores the relative operator precedence of || and &&. This forces the user to place explicit parentheses to group either (a || b) or (b && c) to be evaluated first.

However, GCC warns and Clang is silent about this:

(a || b && 1)

Worse, the same is true for:

(a || b && “foo”)

Now, I find Clang’s rationale seems somewhat reasonable: the grouping doesn’t change the truth table for this expression. However, I find this clever interpretation problematic for several reasons:

  1. It’s a complex rule to teach programmers with dubious gains. This exception doesn’t realistically make the warning’s false positives rare, it just eliminates one common (but not the only common) pattern. On the flip side, it is not a trivial rule to teach or predict for programmers. Any constant that we can evaluate to true will satisfy this.

I’ve never really thought of teaching these rules to people. But maybe people are more habit-driven than I am - I tend to just write the code & stick in the parans when the compiler tells me (or I’m sufficiently confused by the precedence myself - which perhaps comes first so I don’t see these warnings much even though I do tend to write rather dense code that can include many conditions).

  1. If the programmer expected the || to be evaluated first, they might well also expect the “a” expression to be evaluated first and short circuiting to take place. Even though the truth table is unimpacted, ‘assert(!ptr || ptr->empty() && “foo”);’ can segfault, which I think might surprise users.

How could that code segfault? the first of the three expressions should cause the shortcircuit no matter which parse occurred.

  1. It makes it annoying to track warnings in both Clang and GCC. If you mostly develop with Clang, but someone else checks out your code with GCC it’ll suddenly start warning (much like it just did in a recent commit.)

Echoing my comments on IRC: Our usual approach is to disable GCC’s warning if this sort of thing comes up. But how did this not fail on our buildbots? Do we already have GCC’s -Wparentheses disabled on them for this or other reasons?

Yes yes, see my follow-up. I just didn't think enough before mashing send.
Ignore this point. =]

Chandler Carruth <chandlerc@gmail.com> writes:

Currently Clang and GCC warn about:

(a || b && c)

Due to the very common mistake by programmers which ignores the relative
operator precedence of || and &&. This forces the user to place explicit
parentheses to group either (a || b) or (b && c) to be evaluated first.

However, GCC warns and Clang is silent about this:

(a || b && 1)

Worse, the same is true for:

(a || b && "foo")

Now, I find Clang's rationale seems somewhat reasonable: the grouping doesn't
change the truth table for this expression. However, I find this clever
interpretation problematic for several reasons:

1) It's a complex rule to teach programmers with dubious gains. This
exception doesn't realistically make the warning's false positives
rare, it just eliminates one common (but not the only common)
pattern. On the flip side, it is not a trivial rule to teach or
predict for programmers. *Any* constant that we can evaluate to true
will satisfy this.

I haven't looked at this code. Does this special case add much/any
complexity?

2) ...

Ignoring... :slight_smile:

3) It makes it annoying to track warnings in both Clang and GCC. If
you mostly develop with Clang, but someone else checks out your code
with GCC it'll suddenly start warning (much like it just did in a
recent commit.)

IMHO, this is the strongest argument. I have no problem with clang
accepting this kind of code, but having to add parentheses here isn't as
bad as having to turn off -Wparentheses. Disabling the warning could
easily read toa real bug when somebody changes something about the
condition in the future. If this also reduces code complexity that's a
win.

No opinion on the actual issue, but…

No opinion on the actual issue, but…

+1

For the ubiquitous assert pattern like:

assert(a || b && “god help us”)

I don’t think this is confusing and the warning offers no value; IMHO adding the parentheses is more like noise than improving clarity.

Is it out-of-the-question to file a report on GCC to see if they would consider also silencing the warning, and in the meantime turn it off for gcc ?

No opinion on the actual issue, but…

Currently Clang and GCC warn about:

(a || b && c)

Due to the very common mistake by programmers which ignores the relative
operator precedence of || and &&. This forces the user to place explicit
parentheses to group either (a || b) or (b && c) to be evaluated first.

However, GCC warns and Clang is silent about this:

(a || b && 1)

Worse, the same is true for:

(a || b && "foo")

Now, I find Clang's rationale seems somewhat reasonable: the grouping
doesn't change the truth table for this expression. However, I find this
clever interpretation problematic for several reasons:

1) It's a complex rule to teach programmers with dubious gains. This
exception doesn't realistically make the warning's false positives rare, it
just eliminates one common (but not the only common) pattern. On the flip
side, it is not a trivial rule to teach or predict for programmers. *Any*
constant that we can evaluate to true will satisfy this.

2) If the programmer expected the || to be evaluated first, they might
well also expect the "a" expression to be evaluated first and short
circuiting to take place. Even though the truth table is unimpacted,
'assert(!ptr || ptr->empty() && "foo");' can segfault, which I think might
surprise users.

3) It makes it annoying to track warnings in both Clang and GCC. If you
mostly develop with Clang, but someone else checks out your code with GCC
it'll suddenly start warning (much like it just did in a recent commit.)

…this isn't a convincing argument. Several of clang's warnings are by
design smarter and less noisy than gcc's, with the effect that the clang
warning is useful while the gcc counterpart isn't
(e.g. Woverloaded-virtual, but there are many more).

+1

For the ubiquitous assert pattern like:

assert(a || b && “god help us”)

I don’t think this is confusing and the warning offers no value; IMHO
adding the parentheses is more like noise than improving clarity.

FWIW, I recall seeing someone somewhere (can't remember when/where/what
context) say something along the lines of "unlike GCC, Clang is smart
enough to ease off on -Wparentheses when inside the assert macro", which I
take to mean that this is a useful behavior.

-- Sean Silva

I have said that more than once. No matter how hard I try, I can never remember to add extra parens in a || b asserts (having spent most of my life using assert macros). Then someone using GCC has to “fix” my code and I feel bad.

In order of not-very-strong preference:

  1. Fix gcc to intelligently ignore this case.

  2. Make clang temporarily dumb for gcc compatibility.

  3. Do nothing.

-Andy