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