On Tue, Apr 16, 2013 at 5:08 PM, Richard Trieu <[email protected]> wrote: > > > > On Tue, Apr 16, 2013 at 3:50 PM, David Blaikie <[email protected]> wrote: >> >> >> 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? > > Millions of lines of Google code.
LGTM then >> >> >> >> >> > >> >> > 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
