r179662 - Add warning group -Woverloaded-shift-op-parentheses to -Wparentheses. This

[Matt Beaumont-Gay]

Apologies for the late review...

On Tue, Apr 16, 2013 at 7:12 PM, Richard Trieu <rtrieu at google.com> wrote:
> Author: rtrieu
> Date: Tue Apr 16 21:12:45 2013
> New Revision: 179662
>
> URL: http://llvm.org/viewvc/llvm-project?rev=179662&view=rev
> Log:
> Add warning group -Woverloaded-shift-op-parentheses to -Wparentheses.  This
> will fire on code such as:
>
>   cout << x == 0;
>
> which the compiler will intrepret as
>
>   (cout << x) == 0;
>
> This warning comes with two fixits attached to notes, one for parentheses to
> silence the warning, and another to evaluate the comparison first.
>
> Modified:
>     cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>     cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>     cfe/trunk/lib/Sema/SemaExpr.cpp
>     cfe/trunk/test/Sema/parentheses.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=179662&r1=179661&r2=179662&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Apr 16 21:12:45 2013
> @@ -122,6 +122,7 @@ def GlobalConstructors : DiagGroup<"glob
>  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">;

Do we really need a separate flag for this? Seems like it would be
fine to include the new warning in -Wshift-op-parentheses.

>  def DanglingElse: DiagGroup<"dangling-else">;
>  def DanglingField : DiagGroup<"dangling-field">;
>  def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
> @@ -352,6 +353,7 @@ def Parentheses : DiagGroup<"parentheses
>                              [LogicalOpParentheses,
>                               BitwiseOpParentheses,
>                               ShiftOpParentheses,
> +                             OverloadedShiftOpParentheses,
>                               ParenthesesOnEquality,
>                               DanglingElse]>;
>
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=179662&r1=179661&r2=179662&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Apr 16 21:12:45 2013
> @@ -4041,6 +4041,13 @@ def warn_bitwise_and_in_bitwise_or : War
>  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 "

Pretty sure this should say "overloaded operator ... has *higher*
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>;
>
> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=179662&r1=179661&r2=179662&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Tue Apr 16 21:12:45 2013
> @@ -8877,6 +8877,33 @@ static void DiagnoseAdditionInShift(Sema
>    }
>  }
>
> +static void DiagnoseShiftCompare(Sema &S, SourceLocation OpLoc,
> +                                 Expr *LHSExpr, Expr *RHSExpr) {
> +  CXXOperatorCallExpr *OCE = dyn_cast<CXXOperatorCallExpr>(LHSExpr);
> +  if (!OCE)
> +    return;
> +
> +  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,
> @@ -8905,6 +8932,11 @@ static void DiagnoseBinOpPrecedence(Sema
>      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.
>
> Modified: cfe/trunk/test/Sema/parentheses.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/parentheses.cpp?rev=179662&r1=179661&r2=179662&view=diff
> ==============================================================================
> --- cfe/trunk/test/Sema/parentheses.cpp (original)
> +++ cfe/trunk/test/Sema/parentheses.cpp Tue Apr 16 21:12:45 2013
> @@ -29,6 +29,12 @@ void f(Stream& s, bool b) {
>    (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 {
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits