On Apr 17, 2013 6:12 AM, "Richard Trieu" <[email protected]> wrote: > > > > > On Tue, Apr 16, 2013 at 6:40 AM, David Blaikie <[email protected]> wrote: >> >> >> On Apr 10, 2013 10:04 AM, "Richard Trieu" <[email protected]> wrote: >> > >> > Streams commonly overload the << and >> operators for input and output. However, these operators have higher precedence than comparison operators. Code written like this: >> > >> > cout << 5 == 4; >> > >> > Is interpreted as: >> > >> > (cout << 5) == 4; >> > >> > Instead of: >> > >> > cout << (5 == 4); >> >> 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,. > > 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.
What sort of/how much code have you treated it on? >> >> > >> > http://llvm-reviews.chandlerc.com/D650 >> > >> > Files: >> > lib/Sema/SemaExpr.cpp >> > test/Sema/parentheses.cpp >> > include/clang/Basic/DiagnosticSemaKinds.td >> > include/clang/Basic/DiagnosticGroups.td >> > >> > Index: lib/Sema/SemaExpr.cpp >> > =================================================================== >> > --- lib/Sema/SemaExpr.cpp >> > +++ lib/Sema/SemaExpr.cpp >> > @@ -8819,6 +8819,27 @@ >> > } >> > } >> > >> > +static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc, >> > + Expr *LHSExpr, Expr *RHSExpr) { >> > + CXXOperatorCallExpr *OCE = dyn_cast<CXXOperatorCallExpr>(LHSExpr); >> > + if (!OCE) return; >> >> 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) >> >> > + FunctionDecl *FD = OCE->getDirectCallee(); >> > + if (!FD || !FD->isOverloadedOperator()) return; >> > + OverloadedOperatorKind Kind = FD->getOverloadedOperator(); >> > + if (Kind != OO_LessLess && Kind != OO_GreaterGreater) return; >> > + S.Diag(OpLoc, diag::warn_overloaded_shift_in_comparison) >> > + << LHSExpr->getSourceRange() << RHSExpr->getSourceRange() >> > + << (Kind == OO_LessLess); >> > + SuggestParentheses(S, OpLoc, >> > + S.PDiag(diag::note_evaluate_comparison_first), >> > + SourceRange(OCE->getArg(1)->getLocStart(), >> > + RHSExpr->getLocEnd())); >> > + SuggestParentheses(S, OCE->getOperatorLoc(), >> > + S.PDiag(diag::note_precedence_silence) >> > + << (Kind == OO_LessLess ? "<<" : ">>"), >> > + OCE->getSourceRange()); >> > +} >> > + >> > /// DiagnoseBinOpPrecedence - Emit warnings for expressions with tricky >> > /// precedence. >> > static void DiagnoseBinOpPrecedence(Sema &Self, BinaryOperatorKind Opc, >> > @@ -8847,6 +8868,11 @@ >> > DiagnoseAdditionInShift(Self, OpLoc, LHSExpr, Shift); >> > DiagnoseAdditionInShift(Self, OpLoc, RHSExpr, Shift); >> > } >> > + >> > + // Warn on overloaded shift operators and comparisons, such as: >> > + // cout << 5 == 4; >> > + if (BinaryOperator::isComparisonOp(Opc)) >> > + DiagnoseShiftCompare(Self, OpLoc, LHSExpr, RHSExpr); >> > } >> > >> > // Binary Operators. 'Tok' is the token for the operator. >> > Index: test/Sema/parentheses.cpp >> > =================================================================== >> > --- test/Sema/parentheses.cpp >> > +++ test/Sema/parentheses.cpp >> > @@ -29,6 +29,12 @@ >> > (void)(s << b ? "foo" : "bar"); // expected-warning {{operator '?:' has lower precedence than '<<'}} \ >> > // expected-note {{place parentheses around the '?:' expression to evaluate it first}} \ >> > // expected-note {{place parentheses around the '<<' expression to silence this warning}} >> > + (void)(s << 5 == 1); // expected-warning {{overloaded operator << has lower precedence than comparison operator}} \ >> > + // expected-note {{place parentheses around the '<<' expression to silence this warning}} \ >> > + // expected-note {{place parentheses around comparison expression to evaluate it first}} >> > + (void)(s >> 5 == 1); // expected-warning {{overloaded operator >> has lower precedence than comparison operator}} \ >> > + // expected-note {{place parentheses around the '>>' expression to silence this warning}} \ >> > + // expected-note {{place parentheses around comparison expression to evaluate it first}} >> > } >> > >> > struct S { >> > Index: include/clang/Basic/DiagnosticSemaKinds.td >> > =================================================================== >> > --- include/clang/Basic/DiagnosticSemaKinds.td >> > +++ include/clang/Basic/DiagnosticSemaKinds.td >> > @@ -4025,6 +4025,13 @@ >> > def warn_logical_and_in_logical_or : Warning< >> > "'&&' within '||'">, InGroup<LogicalOpParentheses>; >> > >> > +def warn_overloaded_shift_in_comparison :Warning< >> > + "overloaded operator %select{>>|<<}0 has lower precedence than " >> > + "comparison operator">, >> > + InGroup<OverloadedShiftOpParentheses>; >> > +def note_evaluate_comparison_first :Note< >> > + "place parentheses around comparison expression to evaluate it first">; >> > + >> > def warn_addition_in_bitshift : Warning< >> > "operator '%0' has lower precedence than '%1'; " >> > "'%1' will be evaluated first">, InGroup<ShiftOpParentheses>; >> > Index: include/clang/Basic/DiagnosticGroups.td >> > =================================================================== >> > --- include/clang/Basic/DiagnosticGroups.td >> > +++ include/clang/Basic/DiagnosticGroups.td >> > @@ -122,6 +122,7 @@ >> > def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">; >> > def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">; >> > def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">; >> > +def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">; >> > def DanglingElse: DiagGroup<"dangling-else">; >> > def DanglingField : DiagGroup<"dangling-field">; >> > def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">; >> > @@ -352,6 +353,7 @@ >> > [LogicalOpParentheses, >> > BitwiseOpParentheses, >> > ShiftOpParentheses, >> > + OverloadedShiftOpParentheses, >> > ParenthesesOnEquality, >> > DanglingElse]>; >> > >> > _______________________________________________ >> > cfe-commits mailing list >> > [email protected] >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
