[cfe-dev] Fwd: [cfe-commits] r133526 - /cfe/trunk/lib/Sema/SemaExpr.cpp

[Richard Smith]

On Wed, June 22, 2011 00:43, Chandler Carruth wrote:
> On Tue, Jun 21, 2011 at 12:09 PM, Argyrios Kyrtzidis
> <kyrtzidis at apple.com>wrote:
>> On Jun 21, 2011, at 11:45 AM, Chandler Carruth wrote:
>> Moving a conversation from a review thread to cfe-dev for broader
>> comments.
>>
>> Currently Clang has a consistent practice of emitting a fixit
>> suggestion note which silences the warning (without changing semantics)
>> first, followed by any alternative suggestions. This seems a bit counter
>> intuitive, as in many (most?) cases, the alternative suggestion is more
>> likely what the user intended.
>>
>> Can you offer specific examples ?
>
> Sure.
>
> -Wparentheses for ?: precedence: After looking at 50+ errors of this
> form, I've yet to find a single one where the correct suggestion is to
> silence the warning with ()s around the condition expression ("(a + b) ? x
> : y"); all of
> them were intended to be "a + (b ? x : y)".
>
> -Wparentheses for assignment in an if-statement's condition expression:
> While when first turning this warning on there are plenty of cases where
> the correct change is to add extra ()s to silence the warning, once done
> and the engineering practices established to always use double parentheses
> when assigning, invariably I see more people forgetting the second '='
> than forgetting the extra parentheses.
>
> -Wparentheses for equality test in an if-statement with extra
> parentheses:
> (same as above)
>
> I think 'silence' operation is about equally likely to be the correct
> response for
>
> -Wparentheses for mixed && and ||
> -Wparentheses for mixed & and |

When we turned this warning on for our codebase, we found only one bug
among about 20 warnings.

> I think these are the major warnings we have with a multiple fixit
> suggestions where one suggestion is "silence"ing, but I haven't been
> terribly thorough in my search.
>
>> After discussing this on IRC with dgregor, he agreed and raised another
>>  perspective that I find compelling for inverting the current policy:
>> if there is a fixit suggestion note which merely silences the compiler,
>> it should be *last*.
>>
>> < dgregor> I do agree that always putting "silence the compiler" last
>> would be better? essentially, one could imagine the user reading each of
>> the notes, shaking his head, and then clicking on the last one "oh, shut
>> up, I know what I'm doing"
>>
>> I'd personally would like the one that was more likely what I intended
>> to be first.
>
> Personally, I agree. However, I see Doug's point about consistency of UI
> also being nice. It's especially nice in the context of a graphical UI.
>
> I also really like Doug's perspective. We should strive for warnings
> where we *expect* the least likely candidate to be silencing the compiler.
> If
> that's the common response, it seems like a problem with the warning. The
> whole point of why reading through the alternatives presented by the
> compiler before the option of silencing the warning is that for this code
>  pattern there is some reason (even if just historical patterns such as
> "if
> (x = y)") to believe that one of the non-silencing options is what the
> programmer intended to write.
>
> As it happens, I think our current warnings fit this model. I'd not be
> displeased to see the 'silence' option last for any of the ones I've
> looked at.

For the && versus || warning, 'silence the warning' is presumably the most
likely response (since everyone should get it right /at least/ half the
time). I don't think we should strive to avoid such warnings (but I'm not
fussed about the order the notes appear for that one.)

Richard