[PATCH] Add new warning to -Wparentheses for mixing overloaded shift operators and comparison operators

[David Blaikie]

On Tue, Apr 16, 2013 at 5:08 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>
>
> 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.

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
>> >> > cfe-commits at cs.uiuc.edu
>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >> >
>> >
>> >
>
>