<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Apr 16, 2013 at 6:40 AM, 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 class="im"><p dir="ltr"><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);</p>
</div><p dir="ltr">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,.</p></blockquote><div style>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.<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
<p dir="ltr">><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;</p>
</div><p dir="ltr">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)</p>
<p dir="ltr"></p><div><div class="h5">> + 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></div></div>
> _______________________________________________<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>
<p></p>
</blockquote></div><br></div></div>