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

[Chandler Carruth]

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 |

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110621/d8f143a2/attachment.html>