[PATCH] Add new warning to -Wparentheses for mixing overloaded shift operators and comparison operators
Richard Trieu
rtrieu at google.com
Tue Apr 16 17:08:14 PDT 2013
On Tue, Apr 16, 2013 at 3:50 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> On Apr 17, 2013 6:12 AM, "Richard Trieu" <rtrieu at google.com> wrote:
> >
> >
> >
> >
> > On Tue, Apr 16, 2013 at 6:40 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >>
> >> On Apr 10, 2013 10:04 AM, "Richard Trieu" <rtrieu at google.com> 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
> >> > cfe-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130416/cbea3b32/attachment.html>
More information about the cfe-commits
mailing list