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

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.

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”

Thoughts? If there is general agreement with the strategy, I’m willing to revisit my patch, implementing the policy consistently across all of our warnings.
-Chandler

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 ?

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.

-Argiris

I agree in theory, but I think that determining "what's most likely" is more nebulous than "silence the warning without changing semantics". I don't particularly mind whether "silence" comes first or last, but I think it's more important to have a convention for that than it is to get the /first/ fixit to be the most useful.

The way I personally see fixits as "the standard assumes you meant this, but maybe you meant this". Seen that way it makes sense to put the "silence" option first. On the other hand, if an ambiguous expression gets flagged, maybe it's a good thing if the compiler makes me think about what I meant, which would support putting the "silence" option at the end.

Jordy

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.

I agree in theory, but I think that determining "what's most likely" is more nebulous than "silence the warning without changing semantics". I don't particularly mind whether "silence" comes first or last, but I think it's more important to have a convention for that than it is to get the /first/ fixit to be the most useful.

I don't think most users realize there is a convention, when they complain about the order it's because they want the one that they intended first. If "what's most likely" is nebulous the just use the convention.
Anyway, I'm all for consistency, I'm pointing out that users just care for less friction :slight_smile:

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.

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.

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

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 this is very similar to the assignment-in-condition-expression warning: there is a non-trivial “false-positive” rate when you first turn it on because it’s essentially asking for extra clarification on a dangerous construct. I put false-positive in quotes, because with both the presumption is that this is a sufficiently confusing construct to merit clarification from the programmer in either direction.

For both of these, the real question is what the false positive rate is once widely used as people do development…

For the && versus || warning, ‘silence the warning’ is presumably the most

likely response (since everyone should get it right /at least/ half the
time).

There are lots of factors here. I actually view this warning as “I didn’t group these explicitly” so its always a problem in my code. The reader will be confused otherwise even if the program happens to behave correctly.

Also, given that pattern, I tend to always group them explicitly. I suspect this is a fairly common trend.

Once that trend is established, the most common way I encounter this warning is by changing a set of logical operations without re-grouping them. In these cases I really do think its completely random whether or not the change breaks the code.

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.)

Sure, but it’s important to note that such warnings do have a high bar: it has to be reasonable to require an explicit clarification of intent, even if the behavior would have been correct. That’s not to say we shouldn’t have such warnings, just that they need to be suitably justified.