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

[David Blaikie]

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?

>>
>> >
>> > 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/54a696bb/attachment.html>