<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 16, 2013 at 3:50 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><p dir="ltr"><br>
On Apr 17, 2013 6:12 AM, "Richard Trieu" <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
><br>
><br>
><br>
><br>
> On Tue, Apr 16, 2013 at 6:40 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> On Apr 10, 2013 10:04 AM, "Richard Trieu" <<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>> wrote:<br>
>> ><br>
>> > Streams commonly overload the << and >> operators for input and output. However, these operators have higher precedence than comparison operators. Code written like this:<br>
>> ><br>
>> > cout << 5 == 4;<br>
>> ><br>
>> > Is interpreted as:<br>
>> ><br>
>> > (cout << 5) == 4;<br>
>> ><br>
>> > Instead of:<br>
>> ><br>
>> > cout << (5 == 4);<br>
>><br>
>> What sort of statistics on success rate do you have with this warning? I wonder if it would find many intentional uses when dealing with numeric user defined tyows,.<br>
><br>
> Prior to this patch, two reports of this type of bug were found and fixed. This warning has not triggered on any other code I've tested, true or false positive.</p>
</div><p dir="ltr">What sort of/how much code have you treated it on?</p></blockquote><div style>Millions of lines of Google code. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
<div>
<p dir="ltr">>><br>
>> ><br>
>> > <a href="http://llvm-reviews.chandlerc.com/D650" target="_blank">http://llvm-reviews.chandlerc.com/D650</a><br>
>> ><br>
>> > Files:<br>
>> > lib/Sema/SemaExpr.cpp<br>
>> > test/Sema/parentheses.cpp<br>
>> > include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > include/clang/Basic/DiagnosticGroups.td<br>
>> ><br>
>> > Index: lib/Sema/SemaExpr.cpp<br>
>> > ===================================================================<br>
>> > --- lib/Sema/SemaExpr.cpp<br>
>> > +++ lib/Sema/SemaExpr.cpp<br>
>> > @@ -8819,6 +8819,27 @@<br>
>> > }<br>
>> > }<br>
>> ><br>
>> > +static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc,<br>
>> > + Expr *LHSExpr, Expr *RHSExpr) {<br>
>> > + CXXOperatorCallExpr *OCE = dyn_cast<CXXOperatorCallExpr>(LHSExpr);<br>
>> > + if (!OCE) return;<br>
>><br>
>> These early returns seem a little hard to spot in a fairly dense/flat piece if code. Might benefit from putting the return on their own line and/or some comments (or perhaps this is easy enough to read with syntax highlighting)<br>
>><br>
>> > + FunctionDecl *FD = OCE->getDirectCallee();<br>
>> > + if (!FD || !FD->isOverloadedOperator()) return;<br>
>> > + OverloadedOperatorKind Kind = FD->getOverloadedOperator();<br>
>> > + if (Kind != OO_LessLess && Kind != OO_GreaterGreater) return;<br>
>> > + S.Diag(OpLoc, diag::warn_overloaded_shift_in_comparison)<br>
>> > + << LHSExpr->getSourceRange() << RHSExpr->getSourceRange()<br>
>> > + << (Kind == OO_LessLess);<br>
>> > + SuggestParentheses(S, OpLoc,<br>
>> > + S.PDiag(diag::note_evaluate_comparison_first),<br>
>> > + SourceRange(OCE->getArg(1)->getLocStart(),<br>
>> > + RHSExpr->getLocEnd()));<br>
>> > + SuggestParentheses(S, OCE->getOperatorLoc(),<br>
>> > + S.PDiag(diag::note_precedence_silence)<br>
>> > + << (Kind == OO_LessLess ? "<<" : ">>"),<br>
>> > + OCE->getSourceRange());<br>
>> > +}<br>
>> > +<br>
>> > /// DiagnoseBinOpPrecedence - Emit warnings for expressions with tricky<br>
>> > /// precedence.<br>
>> > static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc,<br>
>> > @@ -8847,6 +8868,11 @@<br>
>> > DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift);<br>
>> > DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift);<br>
>> > }<br>
>> > +<br>
>> > + // Warn on overloaded shift operators and comparisons, such as:<br>
>> > + // cout << 5 == 4;<br>
>> > + if (BinaryOperator::isComparisonOp(Opc))<br>
>> > + DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr);<br>
>> > }<br>
>> ><br>
>> > // Binary Operators. 'Tok' is the token for the operator.<br>
>> > Index: test/Sema/parentheses.cpp<br>
>> > ===================================================================<br>
>> > --- test/Sema/parentheses.cpp<br>
>> > +++ test/Sema/parentheses.cpp<br>
>> > @@ -29,6 +29,12 @@<br>
>> > (void)(s << b ? "foo" : "bar"); // expected-warning {{operator '?:' has lower precedence than '<<'}} \<br>
>> > // expected-note {{place parentheses around the '?:' expression to evaluate it first}} \<br>
>> > // expected-note {{place parentheses around the '<<' expression to silence this warning}}<br>
>> > + (void)(s << 5 == 1); // expected-warning {{overloaded operator << has lower precedence than comparison operator}} \<br>
>> > + // expected-note {{place parentheses around the '<<' expression to silence this warning}} \<br>
>> > + // expected-note {{place parentheses around comparison expression to evaluate it first}}<br>
>> > + (void)(s >> 5 == 1); // expected-warning {{overloaded operator >> has lower precedence than comparison operator}} \<br>
>> > + // expected-note {{place parentheses around the '>>' expression to silence this warning}} \<br>
>> > + // expected-note {{place parentheses around comparison expression to evaluate it first}}<br>
>> > }<br>
>> ><br>
>> > struct S {<br>
>> > Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > ===================================================================<br>
>> > --- include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > +++ include/clang/Basic/DiagnosticSemaKinds.td<br>
>> > @@ -4025,6 +4025,13 @@<br>
>> > def warn_logical_and_in_logical_or : Warning<<br>
>> > "'&&' within '||'">, InGroup<LogicalOpParentheses>;<br>
>> ><br>
>> > +def warn_overloaded_shift_in_comparison :Warning<<br>
>> > + "overloaded operator %select{>>|<<}0 has lower precedence than "<br>
>> > + "comparison operator">,<br>
>> > + InGroup<OverloadedShiftOpParentheses>;<br>
>> > +def note_evaluate_comparison_first :Note<<br>
>> > + "place parentheses around comparison expression to evaluate it first">;<br>
>> > +<br>
>> > def warn_addition_in_bitshift : Warning<<br>
>> > "operator '%0' has lower precedence than '%1'; "<br>
>> > "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>;<br>
>> > Index: include/clang/Basic/DiagnosticGroups.td<br>
>> > ===================================================================<br>
>> > --- include/clang/Basic/DiagnosticGroups.td<br>
>> > +++ include/clang/Basic/DiagnosticGroups.td<br>
>> > @@ -122,6 +122,7 @@<br>
>> > def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;<br>
>> > def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;<br>
>> > def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;<br>
>> > +def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;<br>
>> > def DanglingElse: DiagGroup<"dangling-else">;<br>
>> > def DanglingField : DiagGroup<"dangling-field">;<br>
>> > def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;<br>
>> > @@ -352,6 +353,7 @@<br>
>> > [LogicalOpParentheses,<br>
>> > BitwiseOpParentheses,<br>
>> > ShiftOpParentheses,<br>
>> > + OverloadedShiftOpParentheses,<br>
>> > ParenthesesOnEquality,<br>
>> > DanglingElse]>;<br>
>> ><br>
>> > _______________________________________________<br>
>> > cfe-commits mailing list<br>
>> > <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
>> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> ><br>
><br>
><br>
</p>
</div></div></blockquote></div><br></div></div>