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

[Argyrios Kyrtzidis]

On Jun 21, 2011, at 4:43 PM, 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 |
> 
> 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.

Right, excellent point.

> 
> 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.
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110621/32587fb4/attachment.html>