[clang-tidy] Cover cases like (b && c && b) in the redundant expression check

[alexeyr]

Summary

readability-redundant-expression could detect expressions where a logical or bitwise operator had equivalent LHS and RHS, but not ones where equivalent operands were separated by more operands.

[alexeyr]

Fixed test broken by git clang-format

[alexeyr]

Note: during testing I found that ++X && ++X is a false positive, my current implementation also introduces X && ++X && X (fixed in the last update).

[alexeyr]

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

335

Please don;e use else after return.

345

Could it be const auto *?

347

Please don't use auto when type is not spelled explicitly or iterator.

352

Please elide braces.

363

Please don't use auto when type is not spelled explicitly or iterator.

366

Please don't use auto when type is not spelled explicitly or iterator.

375

Please elide braces.

402

Please elide braces.

1229

Please elide braces.

[alexeyr]

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

363

In this case the type will depend on TExpr: either BinaryOperator::Opcode or OverloadedOperatorKind. I could make it a template parameter (but it won't be deducible) or convert OverloadedOperatorKind to Opcode. Any preference?

375

Should braces be elided from for as well?

[alexeyr]

Followed Eugene Zelenko's comments and fixed a false positive.

[alexeyr]

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

363

Converted both to OverloadedOperatorKind (because I have a use for OO_None).

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

114

What do you think about the following?

bool foo(int&);
bool bar();

int i;
if (foo(i) && bar() && foo(i)) return 1;

I think that this should not be warned on (under the assumption that the reference variable can be modified by the call and thus may or may not be duplicate), but didn't see a test covering it.

It also brings up an interesting question about what to do if those were non-const pointers rather than references, because the data being pointed to could be modified as well.

(If you think this should be done separately from this review, that's totally fine by me, it looks like it would be an issue with the original code as well.)

One thing that is missing from this review are tests for the overloaded operator functionality.

[alexeyr]

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

114

This is actually handled correctly, by the same logic as (X && X++ && X), so I don't think it needs a separate test. The drawback is that:

1. it's too conservative, X && bar() && X isn't warned on either, because I don't know a way to check that bar() doesn't have side effects on X and so just test HasSideEffects (https://stackoverflow.com/questions/60035219/check-which-variables-can-be-side-effected-by-expression-evaluation-in-clang-ast).

2. the original code does have the same issue and I didn't fix it, so foo(X) && foo(X) and X++ && X++ do get a warning.

I'll add overloaded operator tests.

[aaron.ballman]

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

114

Okay, that seems reasonable to me, thank you!

[alexeyr]

Added tests for overloaded operators and fixed parent check logic.

[alexeyr]

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

114

I've added the tests (which uncovered a problem not limited to overloaded operators; I needed to skip uninteresting nodes when looking at parents as well).

[alexeyr]

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

114

Do you have any insight on https://reviews.llvm.org/D73775#1851553?

[aaron.ballman]

In D73775#1851553, @alexeyr wrote:

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

[alexeyr]

In D73775#1856765, @aaron.ballman wrote:

In D73775#1851553, @alexeyr wrote:

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

[aaron.ballman]

In D73775#1856848, @alexeyr wrote:

In D73775#1856765, @aaron.ballman wrote:

In D73775#1851553, @alexeyr wrote:

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

1224

Diagnostics in clang tidy are not capitalized, so this should be overloaded operator and operator.

[alexeyr]

In D73775#1856869, @aaron.ballman wrote:

In D73775#1856848, @alexeyr wrote:

In D73775#1856765, @aaron.ballman wrote:

In D73775#1851553, @alexeyr wrote:

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

After stepping through diagnostics code, I... don't understand how it is supposed to work. ClangTidyDiagnosticConsumer is

A diagnostic consumer that turns each Diagnostic into a SourceManager-independent ClangTidyError.

And it adds the hints to the ClangTidyError here:

for (const auto &FixIt : Hints) { ... }

But not the ranges, and I don't see any place where it does or even how they can be inserted into a ClangTidyError in the first place!

[aaron.ballman]

In D73775#1859156, @alexeyr wrote:

In D73775#1856869, @aaron.ballman wrote:

In D73775#1856848, @alexeyr wrote:

In D73775#1856765, @aaron.ballman wrote:

In D73775#1851553, @alexeyr wrote:

Also I am not sure why, but the ranges added to the diagnostic in lines 1222-1226 don't show up in the message.

Do you mean that there are no squiggly underlines under the range, or something else? Passing in a range to the diagnostics engine gives it something to put an underline under, as in what's under the -12 in: https://godbolt.org/z/GeQzYg

Yes, exactly. I expect to see the underlines, but don't; only the ^ in the location provided to diag call.

The only time I've seen that happen is when the range is invalid. I'm guessing you'll have to step through the diagnostics code to see what's going on.

After stepping through diagnostics code, I... don't understand how it is supposed to work. ClangTidyDiagnosticConsumer is

A diagnostic consumer that turns each Diagnostic into a SourceManager-independent ClangTidyError.

And it adds the hints to the ClangTidyError here:

for (const auto &FixIt : Hints) { ... }

But not the ranges, and I don't see any place where it does or even how they can be inserted into a ClangTidyError in the first place!

Huh, this does seem like it may be a bug in clang-tidy. I would have expected ClangTidyDiagnosticConsumer::forwardDiagnostic() to do this work.

[alexeyr]

Lower-case warning messages

[alexeyr]

In D73775#1859167, @aaron.ballman wrote:

Huh, this does seem like it may be a bug in clang-tidy. I would have expected ClangTidyDiagnosticConsumer::forwardDiagnostic() to do this work.

From my debugging attempt it seems not to be called (it's only supposed to be "If there is an external diagnostics engine, like in the ClangTidyPluginAction case"). Reported at https://bugs.llvm.org/show_bug.cgi?id=44799.

[alexeyr]

Ping. Are any other changes needed?

[aaron.ballman]

LGTM aside from a minor nit.

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

355

No else after a return.

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

195

I think that this is reasonable behavior, however, once the user overloads the operator there's no way to know whether calling that operator has other side effects that make this not actually be equivalent to U && V. That said, unless this happens an awful lot in practice, this still seems like a reasonable heuristic.

[alexeyr]

@aaron.ballman Updated.

[alexeyr]

Actually updated.

[aaron.ballman]

Still LGTM, thank you! If you need me to commit this on your behalf, I'm happy to do so, just let me know. It'll have to wait until next week, though (unless someone else does it first).

[alexeyr]

Yes, I do. Thank you (and @Eugene.Zelenko) for your help making this better!

[alexeyr]

@aaron.ballman I just noticed this in https://mlir.llvm.org/getting_started/Contributing/

Note: if you haven’t used Arcanist to send your patch for review, committers don’t have access to your preferred identity for commit messages. Make sure to communicate it to them through available channels or use the git sign-off functionality to make your identity visible in the commit message.

Can you please make sure the email is listed as [e-mail] when committing (I've just set it to primary, so it may be automatic?)

[aaron.ballman]

In D73775#1875921, @alexeyr wrote:

@aaron.ballman I just noticed this in https://mlir.llvm.org/getting_started/Contributing/

Note: if you haven’t used Arcanist to send your patch for review, committers don’t have access to your preferred identity for commit messages. Make sure to communicate it to them through available channels or use the git sign-off functionality to make your identity visible in the commit message.

Can you please make sure the email is listed as [e-mail] when committing (I've just set it to primary, so it may be automatic?)

Absolutely -- thank you for clarifying what email address you wanted used. I've commit on your behalf in 5e7d0ebf735a8b70f92acd1f91c7c45423e611cc

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
+#include "llvm/ADT/SmallBitVector.h"
 #include "llvm/Support/Casting.h"
+#include "llvm/Support/FormatVariadic.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <string>
 #include <vector>
 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::matchers;

 static void transformSubToCanonicalAddExpr(BinaryOperatorKind &Opcode,
                                            APSInt &Value) {
   if (Opcode == BO_Sub) {
     Opcode = BO_Add;
     Value = -Value;
   }
 }
 
+// to use in the template below
+static OverloadedOperatorKind getOp(const BinaryOperator *Op) {
+  return BinaryOperator::getOverloadedOperator(Op->getOpcode());
+}
+
+static OverloadedOperatorKind getOp(const CXXOperatorCallExpr *Op) {
+  if (Op->getNumArgs() != 2)
+    return OO_None;
+  return Op->getOperator();
+}
+
+static std::pair<const Expr *, const Expr *>
+getOperands(const BinaryOperator *Op) {
+  return {Op->getLHS()->IgnoreParenImpCasts(),
+          Op->getRHS()->IgnoreParenImpCasts()};
+}
+
+static std::pair<const Expr *, const Expr *>
+getOperands(const CXXOperatorCallExpr *Op) {
+  return {Op->getArg(0)->IgnoreParenImpCasts(),
+          Op->getArg(1)->IgnoreParenImpCasts()};
+}
+
+template <typename TExpr>
+static const TExpr *checkOpKind(const Expr *TheExpr, OverloadedOperatorKind OpKind) {
+  const auto *AsTExpr = dyn_cast_or_null<TExpr>(TheExpr);
+  if (AsTExpr && getOp(AsTExpr) == OpKind)
+    return AsTExpr;
+
+  return nullptr;
+}
+
+// returns true if a subexpression has two directly equivalent operands and
+// is already handled by operands/parametersAreEquivalent
+template <typename TExpr, unsigned N>
+static bool collectOperands(const Expr *Part,
+                            SmallVector<const Expr *, N> &AllOperands,
+                            OverloadedOperatorKind OpKind) {
+  const auto *PartAsBinOp = checkOpKind<TExpr>(Part, OpKind);
+  if (PartAsBinOp) {
+    auto Operands = getOperands(PartAsBinOp);
+    if (areEquivalentExpr(Operands.first, Operands.second))
+      return true;
+    return collectOperands<TExpr>(Operands.first, AllOperands, OpKind) ||
+           collectOperands<TExpr>(Operands.second, AllOperands, OpKind);
+  } else {
+    AllOperands.push_back(Part);
+  }
+  return false;
+}
+
+template <typename TExpr>
+static bool
+markDuplicateOperands(const TExpr *TheExpr,
+                      ast_matchers::internal::BoundNodesTreeBuilder *Builder,
+                      ASTContext &Context) {
+  const OverloadedOperatorKind OpKind = getOp(TheExpr);
+  if (OpKind == OO_None)
+    return false;
+  // if there are no nested operators of the same kind, it's handled by
+  // operands/parametersAreEquivalent
+  const std::pair<const Expr *, const Expr *> Operands = getOperands(TheExpr);
+  if (!(checkOpKind<TExpr>(Operands.first, OpKind) ||
+        checkOpKind<TExpr>(Operands.second, OpKind)))
+    return false;
+
+  // if parent is the same kind of operator, it's handled by a previous call to
+  // markDuplicateOperands
+  const ASTContext::DynTypedNodeList Parents = Context.getParents(*TheExpr);
+  for (ast_type_traits::DynTypedNode Parent : Parents) {
+    if (checkOpKind<TExpr>(Parent.get<TExpr>(), OpKind))
+      return false;
+  }
+
+  SmallVector<const Expr *, 4> AllOperands;
+  if (collectOperands<TExpr>(Operands.first, AllOperands, OpKind))
+    return false;
+  if (collectOperands<TExpr>(Operands.second, AllOperands, OpKind))
+    return false;
+  size_t NumOperands = AllOperands.size();
+  llvm::SmallBitVector Duplicates(NumOperands);
+  for (size_t I = 0; I < NumOperands; I++) {
+    if (Duplicates[I])
+      continue;
+    bool FoundDuplicates = false;
+
+    for (size_t J = I + 1; J < NumOperands; J++) {
+      if (AllOperands[J]->HasSideEffects(Context))
+        break;
+
+      if (areEquivalentExpr(AllOperands[I], AllOperands[J])) {
+        FoundDuplicates = true;
+        Duplicates.set(J);
+        Builder->setBinding(
+            SmallString<11>(llvm::formatv("duplicate{0}", J)),
+            ast_type_traits::DynTypedNode::create(*AllOperands[J]));
+      }
+    }
+
+    if (FoundDuplicates)
+      Builder->setBinding(
+          SmallString<11>(llvm::formatv("duplicate{0}", I)),
+          ast_type_traits::DynTypedNode::create(*AllOperands[I]));
+  }
+
+  return Duplicates.any();
+}
+
 AST_MATCHER(Expr, isIntegerConstantExpr) {
   if (Node.isInstantiationDependent())
     return false;
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
 AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
   return areEquivalentExpr(Node.getLHS(), Node.getRHS());
 }
 
+AST_MATCHER(BinaryOperator, nestedOperandsAreEquivalent) {
+  return markDuplicateOperands(&Node, Builder, Finder->getASTContext());
+}
+
 AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
   return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
 }
 
 AST_MATCHER(CallExpr, parametersAreEquivalent) {
   return Node.getNumArgs() == 2 &&
          areEquivalentExpr(Node.getArg(0), Node.getArg(1));
 }
 
+AST_MATCHER(CXXOperatorCallExpr, nestedParametersAreEquivalent) {
+  return markDuplicateOperands(&Node, Builder, Finder->getASTContext());
+}
+
 AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
   return Node.getOperatorLoc().isMacroID();
 }
 
 AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
   return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
 }
 

 
 // Checks whether the arguments of an overloaded operator can be modified in the
 // function.
 // For operators that take an instance and a constant as arguments, only the
 // first argument (the instance) needs to be checked, since the constant itself
 // is a temporary expression. Whether the second parameter is checked is
 // controlled by the parameter `ParamsToCheckCount`.
 static bool
-canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl,
+canOverloadedOperatorArgsBeModified(const CXXOperatorCallExpr *OperatorCall,
                                     bool checkSecondParam) {
+  const auto *OperatorDecl =
+      dyn_cast_or_null<FunctionDecl>(OperatorCall->getCalleeDecl());
+  // if we can't find the declaration, conservatively assume it can modify
+  // arguments
+  if (!OperatorDecl)
+    return true;
+
   unsigned ParamCount = OperatorDecl->getNumParams();
 
   // Overloaded operators declared inside a class have only one param.
   // These functions must be declared const in order to not be able to modify
   // the instance of the class they are called through.
   if (ParamCount == 1 &&
       !OperatorDecl->getType()->castAs<FunctionType>()->isConst())
     return true;
if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
       return false;
   } else if (const auto *Cast = Result.Nodes.getNodeAs<CastExpr>(CastId)) {
     // Operand received with implicit comparator (cast).
     Opcode = BO_NE;
     OperandExpr = Cast;
     Value = APSInt(32, false);
   } else if (const auto *OverloadedOperatorExpr =
                  Result.Nodes.getNodeAs<CXXOperatorCallExpr>(OverloadId)) {
-    const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(OverloadedOperatorExpr->getCalleeDecl());
-    if (!OverloadedFunctionDecl)
-      return false;
-
-    if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false))
-      return false;
-
-    if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false))
+    if (canOverloadedOperatorArgsBeModified(OverloadedOperatorExpr, false))
       return false;
 
     if (const auto *Arg = OverloadedOperatorExpr->getArg(1)) {
       if (!Arg->isValueDependent() &&
           !Arg->isIntegerConstantExpr(Value, *Result.Context))
         return false;
     }
     Symbol = OverloadedOperatorExpr->getArg(0);
Finder->addMatcher(
                      unless(binaryOperatorIsInMacro()),
                      unless(hasType(realFloatingPointType())),
                      unless(hasEitherOperand(hasType(realFloatingPointType()))),
                      unless(hasLHS(AnyLiteralExpr)),
                      unless(hasDescendant(BannedIntegerLiteral)))
           .bind("binary"),
       this);
 
+  // Logical or bitwise operator with equivalent nested operands, like (X && Y
+  // && X) or (X && (Y && X))
+  Finder->addMatcher(
+      binaryOperator(anyOf(hasOperatorName("|"), hasOperatorName("&"),
+                           hasOperatorName("||"), hasOperatorName("&&"),
+                           hasOperatorName("^")),
+                     nestedOperandsAreEquivalent(),
+                     // Filter noisy false positives.
+                     unless(isInTemplateInstantiation()),
+                     unless(binaryOperatorIsInMacro()),
+                     // TODO: if the banned macros are themselves duplicated
+                     unless(hasDescendant(BannedIntegerLiteral)))
+          .bind("nested-duplicates"),
+      this);
+
   // Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
   Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(),
                                          // Filter noisy false positives.
                                          unless(conditionalOperatorIsInMacro()),
                                          unless(isInTemplateInstantiation()))
                          .bind("cond"),
                      this);
 
Finder->addMatcher(
               hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"),
               hasOverloadedOperatorName("=")),
           parametersAreEquivalent(),
           // Filter noisy false positives.
           unless(isMacro()), unless(isInTemplateInstantiation()))
           .bind("call"),
       this);
 
+  // Overloaded operators with equivalent operands.
+  Finder->addMatcher(cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("|"),
+                                               hasOverloadedOperatorName("&"),
+                                               hasOverloadedOperatorName("||"),
+                                               hasOverloadedOperatorName("&&"),
+                                               hasOverloadedOperatorName("^")),
+                                         nestedParametersAreEquivalent(),
+                                         argumentCountIs(2),
+                                         // Filter noisy false positives.
+                                         unless(isMacro()),
+                                         unless(isInTemplateInstantiation()))
+                         .bind("nested-duplicates"),
+                     this);
+
   // Match expressions like: !(1 | 2 | 3)
   Finder->addMatcher(
       implicitCastExpr(
           hasImplicitDestinationType(isInteger()),
           has(unaryOperator(
                   hasOperatorName("!"),
                   hasUnaryOperand(ignoringParenImpCasts(binaryOperator(
                       anyOf(hasOperatorName("|"), hasOperatorName("&")),
if (const auto *CondOp =
     if (areExprsFromDifferentMacros(TrueExpr, FalseExpr, Result.Context) ||
         areExprsMacroAndNonMacro(TrueExpr, FalseExpr))
       return;
     diag(CondOp->getColonLoc(),
          "'true' and 'false' expressions are equivalent");
   }
 
   if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) {
-    const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(Call->getCalleeDecl());
-    if (!OverloadedFunctionDecl)
-      return;
-
-    if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true))
+    if (canOverloadedOperatorArgsBeModified(Call, true))
       return;
 
     diag(Call->getOperatorLoc(),
          "both sides of overloaded operator are equivalent");
   }
 
+  if (const auto *Op = Result.Nodes.getNodeAs<Expr>("nested-duplicates")) {
+    const auto *Call = dyn_cast<CXXOperatorCallExpr>(Op);
+    if (Call && canOverloadedOperatorArgsBeModified(Call, true))
+      return;
+
+    StringRef Message =
+        Call ? "Overloaded operator has equivalent nested operands"
+             : "Operator has equivalent nested operands";
+
+    const auto Diag = diag(Op->getExprLoc(), Message);
+    for (const auto &KeyValue : Result.Nodes.getMap()) {
+      if (StringRef(KeyValue.first).startswith("duplicate"))
+        Diag << KeyValue.second.getSourceRange();
+    }
+  }
+
   if (const auto *NegateOperator =
           Result.Nodes.getNodeAs<UnaryOperator>("logical-bitwise-confusion")) {
     SourceLocation OperatorLoc = NegateOperator->getOperatorLoc();
 
     auto Diag =
         diag(OperatorLoc,
              "ineffective logical negation operator used; did you mean '~'?");
     SourceLocation LogicalNotLocation = OperatorLoc.getLocWithOffset(1);

clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp

int TestSimpleEquivalent(int X, int Y) {
   if (foo(bar(0)) < (foo(bar((0))))) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both sides of operator are equivalent
 
   if (P1.x < P2.x && P1.x < P2.x) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both sides of operator are equivalent
   if (P2.a[P1.x + 2] < P2.x && P2.a[(P1.x) + (2)] < (P2.x)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
+  if (X && Y && X) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: Operator has equivalent nested operands
+  if (X || (Y || X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: Operator has equivalent nested operands
+
   return 0;
 }
 
 template <int DX>
 int TestSimpleEquivalentDependent() {
   if (DX > 0 && DX > 0) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
int Valid(int X, int Y) {
 
   if (P.a[0] < P.a[0ULL]) return 1;
   if (0 < 0ULL) return 1;
   if ((int)0 < (int)0ULL) return 1;
 
   if (++X != ++X) return 1;
   if (P.a[X]++ != P.a[X]++) return 1;
   if (P.a[X++] != P.a[X++]) return 1;
+  if (X && X++ && X) return 1;
 
   if ("abc" == "ABC") return 1;
   if (foo(bar(0)) < (foo(bat(0, 1)))) return 1;
   return 0;
 }
 
 #define COND_OP_MACRO 9
 #define COND_OP_OTHER_MACRO 9
bool TestOverloadedOperator(MyStruct& S) {
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 
   return true;
 }
 
 #define LT(x, y) (void)((x) < (y))
 #define COND(x, y, z) ((x)?(y):(z))
 #define EQUALS(x, y) (x) == (y)
 
 int TestMacro(int X, int Y) {
   LT(0, 0);
   LT(1, 0);
   LT(X, X);
   LT(X+1, X + 1);
   COND(X < Y, X, X);
   EQUALS(Q, Q);
   return 0;