[clang-tidy] Misc redundant expressions checker updated for macros

[barancsuk]

Summary

Redundant Expression Checker is updated to be able to detect expressions that contain macro constants. Also, other small details are modified to improve the current implementation.

The improvements in detail are as follows:

1.) Binary and ternary operator expressions containing two constants, with at least one of them from a macro, are detected and tested for redundancy.

Macro expressions are treated somewhat differently from other expressions, because the particular values of macros can vary across builds. They can be considered correct and intentional, even if macro values equal, produce ranges that exclude each other or fully overlap, etc. The correctness of a macro expression is decided based on solely the operators involved in the expression.

Examples:

The following expression is always redundant, independently of the macro values, because the subexpression containing the larger constant can be eliminated without changing the meaning of the expression:

(X < MACRO && X < OTHER_MACRO)

On the contrary, the following expression is considered meaningful, because some macro values (e.g.: MACRO = 3, OTHER_MACRO = 2) produce a valid interval:

(X < MACRO && X > OTHER_MACRO)

2.) The code structure is slightly modified: typos are corrected, comments are added and some functions are renamed to improve comprehensibility, both in the checker and the test file. A few test cases are moved to another function.

3.) The checker is now able to detect redundant CXXFunctionalCastExprs as well (it matched only CStyleCastExprs before). A corresponding test case is added.

[xazax.hun]

Thanks for working on this, these additions look very useful to me.

clang-tidy/misc/RedundantExpressionCheck.cpp

124 ↗

(On Diff #118186)

You could merge this case with the above case if you cast Left and Right to ExplicitCastExpr. So both label could end up executing the same code.

330 ↗

(On Diff #118186)

llvm::StringSet might be a more efficient choice.

336 ↗

(On Diff #118186)

You could use count here.

357 ↗

(On Diff #118186)

I think you could just return the pointer and return a null pointer in case it is not an integerConstantExpr. This way no compatibility overload needed.

510 ↗

(On Diff #118186)

I would prefer to return a pair of pointer to be returned to output parameters.

579 ↗

(On Diff #118186)

I think a comment might be good here what do you mean by meaningful.

test/clang-tidy/misc-redundant-expression.cpp

387 ↗

(On Diff #118186)

Why did you remove these test cases?

404 ↗

(On Diff #118186)

Same comment as above.

[JonasToth]

clang-tidy/misc/RedundantExpressionCheck.cpp

357 ↗

(On Diff #118186)

or llvm::Optional making it clear that no result is intended.

[barancsuk]

Address review comments.

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

510 ↗

(On Diff #118186)

I think, since 'Retrieve' functions have multiple output parameters, they should remain unchanged.

'Retrieve' methods are responsible for splitting expressions into parts (e.g.: into opcode and constant integer value) and storing each part into the corresponding output parameter. Therefore it is more reasonable to return whether the operation was successful than returning one of these parameters.

test/clang-tidy/misc-redundant-expression.cpp

387 ↗

(On Diff #118186)

The above test cases were not removed, just moved into the function TestLogical, as they contain only equality operators, and no relational operators.

[xazax.hun]

LGTM! But wait for @aaron.ballman, @alexfh, or @hokein for the final word.

[aaron.ballman]

clang-tidy/misc/RedundantExpressionCheck.cpp

89 ↗

(On Diff #119452)

I would remove the formatting changes from this function.

331 ↗

(On Diff #119452)

This should use a StringRef instead of a std::string to avoid needless copies.

347 ↗

(On Diff #119452)

the values -> the value

355–358 ↗

(On Diff #119452)

This can be reduced to return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);

364 ↗

(On Diff #119452)

The * binds to the identifier, and there's an extra ;.

388 ↗

(On Diff #119452)

I'd go back towards the original formulation: Match a binary operator between a symbolic expression and an integer constant expression.

445 ↗

(On Diff #119452)

I understand the first part of the comment, but not the second part -- why is double negation ineffective?

(Btw, typo: unaffective -> ineffective)

499–500 ↗

(On Diff #119452)

Formatting. Also, this can use const auto * instead of spelling the name twice. Actually, this should be reformulated because you do an isa<> check above. You can replace the if statement and these assignments with:

const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());

if (!LhsBinOp || !RhsBinOp)
  return false

524–525 ↗

(On Diff #119452)

These should use cast<> instead of dyn_cast<> (you've already asserted this property and cast<> will assert anyway).

537 ↗

(On Diff #119452)

Spurious parens.

560–565 ↗

(On Diff #119452)

This can be combined into a single return statement and no local variables.

575–578 ↗

(On Diff #119452)

This can be return LhsLoc.isMacroID() != RhsLoc.isMacroID();

633–635 ↗

(On Diff #119452)

This worries me slightly for macros because those values can be overridden for different compilation targets. So the diagnostic may be correct for a particular configuration of the macros, but incorrect for another. Have you tested this against any large code bases to see what the quality of the diagnostics looks like?

917 ↗

(On Diff #119452)

Spurious newline.

934 ↗

(On Diff #119452)

Spurious newline.

935–936 ↗

(On Diff #119452)

Please don't use auto here as the type is not explicitly spelled out.

950 ↗

(On Diff #119452)

binded? Perhaps you meant bound? (Same comment applies below.)

clang-tidy/misc/RedundantExpressionCheck.h

20 ↗

(On Diff #119452)

uneffective -> ineffective

[barancsuk]

Address review comments, discard ambiguously redundant macro expressions

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

633–635 ↗

(On Diff #119452)

Thank you for your insight!

Now that I come to think of it, although these expressions are redundant for every configuration, there is no adequate simplification for each compilation target, because they might change their behavior if the values vary.

A good example is the following expression, that is always redundant regardless of the values that the macros hold.
However, the particular macro that causes the redundancy can vary from one compilation target to another.

(X < MACRO1 || X < MACRO2)

If MACRO1 > MACRO2, the appropriate simplification is (X < MACRO1 ),
and if MACRO1 < MACRO2, it is (X < MACRO2).

Even expressions, like (X < MACRO1 || X != MACRO2), that can almost always be simplified to (X != MACRO2), change their behavior, if MACRO1 is equal to MACRO2 (then they are always true), and can be considered conscious development choices.

In conclusion, I think, it might be better now to skip these expressions, and check for only the ones that contain the same macro twice, like X < MACRO && X > MACRO.

[aaron.ballman]

clang-tidy/misc/RedundantExpressionCheck.cpp

633–635 ↗

(On Diff #119452)

In conclusion, I think, it might be better now to skip these expressions, and check for only the ones that contain the same macro twice, like X < MACRO && X > MACRO.

I think that approach makes the most sense, at least initially.

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

41 ↗

(On Diff #120820)

Is this block clang formatted? The indentation of the second line looks strange.

[barancsuk]

Fix shifted line

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

41 ↗

(On Diff #120820)

It was, but the second line shifted somehow. I fixed it in the updated diff.

[aaron.ballman]

Aside from some minor commenting nits, this LGTM.

clang-tidy/misc/RedundantExpressionCheck.cpp

377 ↗

(On Diff #121323)

This commenting change is incorrect, the comment should end with a full stop.

445 ↗

(On Diff #119452)

Comment is missing a full stop at the end.

[barancsuk]

Address review comments: fix missing full stops at the end of comments

clang-tools-extra/trunk/clang-tidy/misc/RedundantExpressionCheck.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_REDUNDANT_EXPRESSION_H
 
 #include "../ClangTidy.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
-/// Detect useless or suspicious redundant expressions.
+/// The checker detects expressions that are redundant, because they contain
+/// ineffective, useless parts.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/misc-redundant-expression.html
 class RedundantExpressionCheck : public ClangTidyCheck {
 public:
   RedundantExpressionCheck(StringRef Name, ClangTidyContext *Context)
       : ClangTidyCheck(Name, Context) {}
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;

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

 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
-#include <set>
 #include <string>
 #include <vector>
 
 using namespace clang::ast_matchers;
 using namespace clang::tidy::matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 namespace {
 using llvm::APSInt;
 } // namespace
 
-static const char KnownBannedMacroNames[] =
-    "EAGAIN;EWOULDBLOCK;SIGCLD;SIGCHLD;";
+static const llvm::StringSet<> KnownBannedMacroNames = {"EAGAIN", "EWOULDBLOCK",
+                                                        "SIGCLD", "SIGCHLD"};
 
 static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) {
   Result = Value;
   ++Result;
   return Value < Result;
 }
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
return LeftLit.getBitWidth() == RightLit.getBitWidth() &&
            LeftLit == RightLit;
   }
   case Stmt::FloatingLiteralClass:
     return cast<FloatingLiteral>(Left)->getValue().bitwiseIsEqual(
         cast<FloatingLiteral>(Right)->getValue());
   case Stmt::StringLiteralClass:
     return cast<StringLiteral>(Left)->getBytes() ==
            cast<StringLiteral>(Right)->getBytes();
-
   case Stmt::DependentScopeDeclRefExprClass:
     if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
         cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
       return false;
     return areEquivalentNameSpecifier(
         cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
         cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
   case Stmt::DeclRefExprClass:
     return cast<DeclRefExpr>(Left)->getDecl() ==
            cast<DeclRefExpr>(Right)->getDecl();
   case Stmt::MemberExprClass:
     return cast<MemberExpr>(Left)->getMemberDecl() ==
            cast<MemberExpr>(Right)->getMemberDecl();
-
+  case Stmt::CXXFunctionalCastExprClass:
   case Stmt::CStyleCastExprClass:
-    return cast<CStyleCastExpr>(Left)->getTypeAsWritten() ==
-           cast<CStyleCastExpr>(Right)->getTypeAsWritten();
-
+    return cast<ExplicitCastExpr>(Left)->getTypeAsWritten() ==
+           cast<ExplicitCastExpr>(Right)->getTypeAsWritten();
   case Stmt::CallExprClass:
   case Stmt::ImplicitCastExprClass:
   case Stmt::ArraySubscriptExprClass:
     return true;
-
   case Stmt::UnaryOperatorClass:
     if (cast<UnaryOperator>(Left)->isIncrementDecrementOp())
       return false;
     return cast<UnaryOperator>(Left)->getOpcode() ==
            cast<UnaryOperator>(Right)->getOpcode();
   case Stmt::BinaryOperatorClass:
     return cast<BinaryOperator>(Left)->getOpcode() ==
            cast<BinaryOperator>(Right)->getOpcode();
static bool rangeSubsumesRange(BinaryOperatorKind OpcodeLHS,
   case BO_GE:
     return (OpcodeRHS == BO_GT || OpcodeRHS == BO_GE || OpcodeRHS == BO_EQ) &&
            Comparison <= 0;
   default:
     return false;
   }
 }
 
-static void canonicalNegateExpr(BinaryOperatorKind &Opcode, APSInt &Value) {
+static void transformSubToCanonicalAddExpr(BinaryOperatorKind &Opcode,
+                                           APSInt &Value) {
   if (Opcode == BO_Sub) {
     Opcode = BO_Add;
     Value = -Value;
   }
 }
 
 AST_MATCHER(Expr, isIntegerConstantExpr) {
   if (Node.isInstantiationDependent())
     return false;
   return Node.isIntegerConstantExpr(Finder->getASTContext());
 }
 
-// Returns a matcher for integer constant expression.
+AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
+  return areEquivalentExpr(Node.getLHS(), Node.getRHS());
+}
+
+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(BinaryOperator, binaryOperatorIsInMacro) {
+  return Node.getOperatorLoc().isMacroID();
+}
+
+AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
+  return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
+}
+
+AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); }
+
+AST_MATCHER_P(Expr, expandedByMacro, llvm::StringSet<>, Names) {
+  const SourceManager &SM = Finder->getASTContext().getSourceManager();
+  const LangOptions &LO = Finder->getASTContext().getLangOpts();
+  SourceLocation Loc = Node.getExprLoc();
+  while (Loc.isMacroID()) {
+    StringRef MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
+    if (Names.count(MacroName))
+      return true;
+    Loc = SM.getImmediateMacroCallerLoc(Loc);
+  }
+  return false;
+}
+
+// Returns a matcher for integer constant expressions.
 static ast_matchers::internal::Matcher<Expr>
 matchIntegerConstantExpr(StringRef Id) {
   std::string CstId = (Id + "-const").str();
   return expr(isIntegerConstantExpr()).bind(CstId);
 }
 
-// Retrieve the integer value matched by 'matchIntegerConstantExpr' with name
-// 'Id' and store it into 'Value'.
+// Retrieves the integer expression matched by 'matchIntegerConstantExpr' with
+// name 'Id' and stores it into 'ConstExpr', the value of the expression is
+// stored into `Value`.
 static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result,
-                                        StringRef Id, APSInt &Value) {
+                                        StringRef Id, APSInt &Value,
+                                        const Expr *&ConstExpr) {
   std::string CstId = (Id + "-const").str();
-  const auto *CstExpr = Result.Nodes.getNodeAs<Expr>(CstId);
-  return CstExpr && CstExpr->isIntegerConstantExpr(Value, *Result.Context);
+  ConstExpr = Result.Nodes.getNodeAs<Expr>(CstId);
+  return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);
+}
+
+// Overloaded `retrieveIntegerConstantExpr` for compatibility.
+static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result,
+                                        StringRef Id, APSInt &Value) {
+  const Expr *ConstExpr = nullptr;
+  return retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr);
 }
 
-// Returns a matcher for a symbolic expression (any expression except ingeter
-// constant expression).
+// Returns a matcher for symbolic expressions (matches every expression except
+// ingeter constant expressions).
 static ast_matchers::internal::Matcher<Expr> matchSymbolicExpr(StringRef Id) {
   std::string SymId = (Id + "-sym").str();
   return ignoringParenImpCasts(
       expr(unless(isIntegerConstantExpr())).bind(SymId));
 }
 
-// Retrieve the expression matched by 'matchSymbolicExpr' with name 'Id' and
-// store it into 'SymExpr'.
+// Retrieves the expression matched by 'matchSymbolicExpr' with name 'Id' and
+// stores it into 'SymExpr'.
 static bool retrieveSymbolicExpr(const MatchFinder::MatchResult &Result,
                                  StringRef Id, const Expr *&SymExpr) {
   std::string SymId = (Id + "-sym").str();
   if (const auto *Node = Result.Nodes.getNodeAs<Expr>(SymId)) {
     SymExpr = Node;
     return true;
   }
   return false;
const auto BinOpCstExpr =
                                hasEitherOperand(matchIntegerConstantExpr(Id))),
                 binaryOperator(hasOperatorName("-"),
                                hasLHS(matchSymbolicExpr(Id)),
                                hasRHS(matchIntegerConstantExpr(Id)))))
           .bind(Id);
   return ignoringParenImpCasts(BinOpCstExpr);
 }
 
-// Retrieve sub-expressions matched by 'matchBinOpIntegerConstantExpr' with
+// Retrieves sub-expressions matched by 'matchBinOpIntegerConstantExpr' with
 // name 'Id'.
 static bool
 retrieveBinOpIntegerConstantExpr(const MatchFinder::MatchResult &Result,
                                  StringRef Id, BinaryOperatorKind &Opcode,
                                  const Expr *&Symbol, APSInt &Value) {
   if (const auto *BinExpr = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
     Opcode = BinExpr->getOpcode();
     return retrieveSymbolicExpr(Result, Id, Symbol) &&
            retrieveIntegerConstantExpr(Result, Id, Value);
   }
   return false;
 }
 
-// Matches relational expression: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x).
+// Matches relational expressions: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x).
 static ast_matchers::internal::Matcher<Expr>
 matchRelationalIntegerConstantExpr(StringRef Id) {
   std::string CastId = (Id + "-cast").str();
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
 
   const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
       isComparisonOperator(), expr().bind(Id),
const auto CastExpr =
                        hasSourceExpression(matchSymbolicExpr(Id)))
           .bind(CastId);
 
   const auto NegateRelationalExpr =
       unaryOperator(hasOperatorName("!"),
                     hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))
           .bind(NegateId);
 
+  // Do not bind to double negation.
   const auto NegateNegateRelationalExpr =
       unaryOperator(hasOperatorName("!"),
                     hasUnaryOperand(unaryOperator(
                         hasOperatorName("!"),
                         hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))));
 
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
                NegateNegateRelationalExpr);
 }
 
-// Retrieve sub-expressions matched by 'matchRelationalIntegerConstantExpr' with
+// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr' with
 // name 'Id'.
-static bool
-retrieveRelationalIntegerConstantExpr(const MatchFinder::MatchResult &Result,
-                                      StringRef Id, const Expr *&OperandExpr,
-                                      BinaryOperatorKind &Opcode,
-                                      const Expr *&Symbol, APSInt &Value) {
+static bool retrieveRelationalIntegerConstantExpr(
+    const MatchFinder::MatchResult &Result, StringRef Id,
+    const Expr *&OperandExpr, BinaryOperatorKind &Opcode, const Expr *&Symbol,
+    APSInt &Value, const Expr *&ConstExpr) {
   std::string CastId = (Id + "-cast").str();
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
 
   if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
     // Operand received with explicit comparator.
     Opcode = Bin->getOpcode();
     OperandExpr = Bin;
-    if (!retrieveIntegerConstantExpr(Result, Id, Value))
+
+    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 {
     return false;
   }
 
   if (!retrieveSymbolicExpr(Result, Id, Symbol))
     return false;
 
   if (Result.Nodes.getNodeAs<Expr>(SwapId))
     Opcode = BinaryOperator::reverseComparisonOp(Opcode);
   if (Result.Nodes.getNodeAs<Expr>(NegateId))
     Opcode = BinaryOperator::negateComparisonOp(Opcode);
-
   return true;
 }
 
-AST_MATCHER(BinaryOperator, operandsAreEquivalent) {
-  return areEquivalentExpr(Node.getLHS(), Node.getRHS());
-}
+// Checks for expressions like (X == 4) && (Y != 9)
+static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+  const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
+  const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
+
+  if (!LhsBinOp || !RhsBinOp)
+    return false;
 
-AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
-  return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
+  if ((LhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) ||
+       LhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)) &&
+      (RhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) ||
+       RhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)))
+    return true;
+  return false;
 }
 
-AST_MATCHER(CallExpr, parametersAreEquivalent) {
-  return Node.getNumArgs() == 2 &&
-         areEquivalentExpr(Node.getArg(0), Node.getArg(1));
-}
+// Retrieves integer constant subexpressions from binary operator expressions
+// that have two equivalent sides
+// E.g.: from (X == 5) && (X == 5) retrieves 5 and 5.
+static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
+                                           BinaryOperatorKind &MainOpcode,
+                                           BinaryOperatorKind &SideOpcode,
+                                           const Expr *&LhsConst,
+                                           const Expr *&RhsConst,
+                                           const ASTContext *AstCtx) {
+  assert(areSidesBinaryConstExpressions(BinOp, AstCtx) &&
+         "Both sides of binary operator must be constant expressions!");
+
+  MainOpcode = BinOp->getOpcode();
+
+  const auto *BinOpLhs = cast<BinaryOperator>(BinOp->getLHS());
+  const auto *BinOpRhs = cast<BinaryOperator>(BinOp->getRHS());
+
+  LhsConst = BinOpLhs->getLHS()->isIntegerConstantExpr(*AstCtx)
+                 ? BinOpLhs->getLHS()
+                 : BinOpLhs->getRHS();
+  RhsConst = BinOpRhs->getLHS()->isIntegerConstantExpr(*AstCtx)
+                 ? BinOpRhs->getLHS()
+                 : BinOpRhs->getRHS();
 
-AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
-  return Node.getOperatorLoc().isMacroID();
-}
+  if (!LhsConst || !RhsConst)
+    return false;
 
-AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
-  return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
-}
+  assert(BinOpLhs->getOpcode() == BinOpRhs->getOpcode() &&
+         "Sides of the binary operator must be equivalent expressions!");
 
-AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); }
+  SideOpcode = BinOpLhs->getOpcode();
 
-AST_MATCHER_P(Expr, expandedByMacro, std::set<std::string>, Names) {
-  const SourceManager &SM = Finder->getASTContext().getSourceManager();
-  const LangOptions &LO = Finder->getASTContext().getLangOpts();
-  SourceLocation Loc = Node.getExprLoc();
-  while (Loc.isMacroID()) {
-    std::string MacroName = Lexer::getImmediateMacroName(Loc, SM, LO);
-    if (Names.find(MacroName) != Names.end())
-      return true;
-    Loc = SM.getImmediateMacroCallerLoc(Loc);
-  }
+
+static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
+                                        const Expr *RhsExpr,
+                                        const ASTContext *AstCtx) {
+  if (!LhsExpr || !RhsExpr)
+    return false;
+
+  SourceLocation LhsLoc = LhsExpr->getExprLoc();
+  SourceLocation RhsLoc = RhsExpr->getExprLoc();
+
+  if (!LhsLoc.isMacroID() || !RhsLoc.isMacroID())
+    return false;
+
+  const SourceManager &SM = AstCtx->getSourceManager();
+  const LangOptions &LO = AstCtx->getLangOpts();
+
+  return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) ==
+          Lexer::getImmediateMacroName(RhsLoc, SM, LO));
+}
+
+static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) {
+  if (!LhsExpr || !RhsExpr)
     return false;
+
+  SourceLocation LhsLoc = LhsExpr->getExprLoc();
+  SourceLocation RhsLoc = RhsExpr->getExprLoc();
+
+  return LhsLoc.isMacroID() != RhsLoc.isMacroID();
 }
 
 void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto AnyLiteralExpr = ignoringParenImpCasts(
       anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
 
-  std::vector<std::string> MacroNames =
-      utils::options::parseStringList(KnownBannedMacroNames);
-  std::set<std::string> Names(MacroNames.begin(), MacroNames.end());
-
-  const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(Names));
+  const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(KnownBannedMacroNames));
 
+  // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
       binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"),
                            hasOperatorName("%"), hasOperatorName("|"),
                            hasOperatorName("&"), hasOperatorName("^"),
                            matchers::isComparisonOperator(),
                            hasOperatorName("&&"), hasOperatorName("||"),
                            hasOperatorName("=")),
                      operandsAreEquivalent(),
                      // Filter noisy false positives.
                      unless(isInTemplateInstantiation()),
                      unless(binaryOperatorIsInMacro()),
                      unless(hasType(realFloatingPointType())),
                      unless(hasEitherOperand(hasType(realFloatingPointType()))),
                      unless(hasLHS(AnyLiteralExpr)),
                      unless(hasDescendant(BannedIntegerLiteral)))
           .bind("binary"),
       this);
 
+  // Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
   Finder->addMatcher(
       conditionalOperator(expressionsAreEquivalent(),
                           // Filter noisy false positives.
                           unless(conditionalOperatorIsInMacro()),
-                          unless(hasTrueExpression(AnyLiteralExpr)),
                           unless(isInTemplateInstantiation()))
           .bind("cond"),
       this);
 
+  // Overloaded operators with equivalent operands.
   Finder->addMatcher(
       cxxOperatorCallExpr(
           anyOf(
               hasOverloadedOperatorName("-"), hasOverloadedOperatorName("/"),
               hasOverloadedOperatorName("%"), hasOverloadedOperatorName("|"),
               hasOverloadedOperatorName("&"), hasOverloadedOperatorName("^"),
               hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!="),
               hasOverloadedOperatorName("<"), hasOverloadedOperatorName("<="),
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
 
     if (!retrieveBinOpIntegerConstantExpr(Result, "lhs", LhsOpcode, LhsSymbol,
                                           LhsValue) ||
         !retrieveBinOpIntegerConstantExpr(Result, "rhs", RhsOpcode, RhsSymbol,
                                           RhsValue) ||
         !areEquivalentExpr(LhsSymbol, RhsSymbol))
       return;
 
-    canonicalNegateExpr(LhsOpcode, LhsValue);
-    canonicalNegateExpr(RhsOpcode, RhsValue);
+    transformSubToCanonicalAddExpr(LhsOpcode, LhsValue);
+    transformSubToCanonicalAddExpr(RhsOpcode, RhsValue);
 
     // Check expressions: x + 1 == x + 2  or  x + 1 != x + 2.
     if (LhsOpcode == BO_Add && RhsOpcode == BO_Add) {
       if ((Opcode == BO_EQ && APSInt::compareValues(LhsValue, RhsValue) == 0) ||
           (Opcode == BO_NE && APSInt::compareValues(LhsValue, RhsValue) != 0)) {
         diag(ComparisonOperator->getOperatorLoc(),
              "logical expression is always true");
       } else if ((Opcode == BO_EQ &&
void RedundantExpressionCheck::checkBitwiseExpr(
   }
 }
 
 void RedundantExpressionCheck::checkRelationalExpr(
     const MatchFinder::MatchResult &Result) {
   if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
           "comparisons-of-symbol-and-const")) {
     // Matched expressions are: (x <op> k1) <REL> (x <op> k2).
+    // E.g.: (X < 2) && (X > 4)
     BinaryOperatorKind Opcode = ComparisonOperator->getOpcode();
 
     const Expr *LhsExpr = nullptr, *RhsExpr = nullptr;
-    APSInt LhsValue, RhsValue;
     const Expr *LhsSymbol = nullptr, *RhsSymbol = nullptr;
+    const Expr *LhsConst = nullptr, *RhsConst = nullptr;
     BinaryOperatorKind LhsOpcode, RhsOpcode;
+    APSInt LhsValue, RhsValue;
+
     if (!retrieveRelationalIntegerConstantExpr(
-            Result, "lhs", LhsExpr, LhsOpcode, LhsSymbol, LhsValue) ||
+            Result, "lhs", LhsExpr, LhsOpcode, LhsSymbol, LhsValue, LhsConst) ||
         !retrieveRelationalIntegerConstantExpr(
-            Result, "rhs", RhsExpr, RhsOpcode, RhsSymbol, RhsValue) ||
+            Result, "rhs", RhsExpr, RhsOpcode, RhsSymbol, RhsValue, RhsConst) ||
         !areEquivalentExpr(LhsSymbol, RhsSymbol))
       return;
 
-    // Bring to a canonical form: smallest constant must be on the left side.
+    // Bring expr to a canonical form: smallest constant must be on the left.
     if (APSInt::compareValues(LhsValue, RhsValue) > 0) {
       std::swap(LhsExpr, RhsExpr);
       std::swap(LhsValue, RhsValue);
       std::swap(LhsSymbol, RhsSymbol);
       std::swap(LhsOpcode, RhsOpcode);
     }
 
+    // Constants come from two different macros, or one of them is a macro.
+    if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
+        areExprsMacroAndNonMacro(LhsConst, RhsConst))
+      return;
+
     if ((Opcode == BO_LAnd || Opcode == BO_LOr) &&
         areEquivalentRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
       diag(ComparisonOperator->getOperatorLoc(),
-           "equivalent expression on both side of logical operator");
+           "equivalent expression on both sides of logical operator");
       return;
     }
 
     if (Opcode == BO_LAnd) {
       if (areExclusiveRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
         diag(ComparisonOperator->getOperatorLoc(),
              "logical expression is always false");
       } else if (rangeSubsumesRange(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
if (Opcode == BO_LOr) {
       } else if (rangeSubsumesRange(RhsOpcode, RhsValue, LhsOpcode, LhsValue)) {
         diag(LhsExpr->getExprLoc(), "expression is redundant");
       }
     }
   }
 }
 
 void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
-  if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary"))
-    diag(BinOp->getOperatorLoc(), "both side of operator are equivalent");
-  if (const auto *CondOp = Result.Nodes.getNodeAs<ConditionalOperator>("cond"))
-    diag(CondOp->getColonLoc(), "'true' and 'false' expression are equivalent");
-  if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call"))
+  if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+
+    // If the expression's constants are macros, check whether they are
+    // intentional.
+    if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+      const Expr *LhsConst = nullptr, *RhsConst = nullptr;
+      BinaryOperatorKind MainOpcode, SideOpcode;
+
+      if(!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode, LhsConst,
+                                     RhsConst, Result.Context))
+          return;
+
+      if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
+          areExprsMacroAndNonMacro(LhsConst, RhsConst))
+        return;
+    }
+
+    diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
+  }
+
+  if (const auto *CondOp =
+          Result.Nodes.getNodeAs<ConditionalOperator>("cond")) {
+    const Expr *TrueExpr = CondOp->getTrueExpr();
+    const Expr *FalseExpr = CondOp->getFalseExpr();
+
+    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")) {
     diag(Call->getOperatorLoc(),
-         "both side of overloaded operator are equivalent");
+         "both sides of overloaded operator are equivalent");
+  }
 
+  // Check for the following bound expressions:
+  // - "binop-const-compare-to-sym",
+  // - "binop-const-compare-to-binop-const",
+  // Produced message:
+  // -> "logical expression is always false/true"
   checkArithmeticExpr(Result);
+
+  // Check for the following bound expression:
+  // - "binop-const-compare-to-const",
+  // Produced message:
+  // -> "logical expression is always false/true"
   checkBitwiseExpr(Result);
+
+  // Check for te following bound expression:
+  // - "comparisons-of-symbol-and-const",
+  // Produced messages:
+  // -> "equivalent expression on both sides of logical operator",
+  // -> "logical expression is always false/true"
+  // -> "expression is redundant"
   checkRelationalExpr(Result);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-redundant-expression.rst

 .. title:: clang-tidy - misc-redundant-expression
 
 misc-redundant-expression
 =========================
 
 Detect redundant expressions which are typically errors due to copy-paste.
 
 Depending on the operator expressions may be
 
 - redundant,
 
-- always be ``true``,
+- always ``true``,
 
-- always be ``false``,
+- always ``false``,
 
-- always be a constant (zero or one).
+- always a constant (zero or one).
 
-Example:
+Examples:
 
 .. code-block:: c++
 
   ((x+1) | (x+1))             // (x+1) is redundant
   (p->x == p->x)              // always true
   (p->x < p->x)               // always false
   (speed - speed + 1 == 12)   // speed - speed is always zero

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

 
 extern Point P1;
 extern Point P2;
 
 extern int foo(int x);
 extern int bar(int x);
 extern int bat(int x, int y);
 
-int Test(int X, int Y) {
+int TestSimpleEquivalent(int X, int Y) {
   if (X - X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent [misc-redundant-expression]
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent [misc-redundant-expression]
   if (X / X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X % X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X & X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X | X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X ^ X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X < X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X <= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X > X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X >= X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
   if (X || X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X != (((X)))) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of operator are equivalent
 
   if (X + 1 == X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 != X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 <= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X + 1 >= X + 1) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
 
   if ((X != 1 || Y != 1) && (X != 1 || Y != 1)) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
   if (P.a[X - P.x] != P.a[X - P.x]) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: both sides of operator are equivalent
 
   if ((int)X < (int)X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
+  if (int(X) < int(X)) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if ( + "dummy" == + "dummy") return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both sides of operator are equivalent
   if (L"abc" == L"abc") return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
 
   if (foo(0) - 2 < foo(0) - 2) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: both sides of operator are equivalent
   if (foo(bar(0)) < (foo(bar((0))))) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: both side of operator are equivalent
+  // 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 side of operator are equivalent
+  // 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 side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: both sides of operator are equivalent
 
   return 0;
 }
 
 int Valid(int X, int Y) {
   if (X != Y) return 1;
   if (X == Y + 0) return 1;
   if (P.x == P.y) return 1;
   if (P.a[P.x] < P.a[P.y]) return 1;
   if (P.a[0] < P.a[1]) return 1;
 
   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 ("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
 int TestConditional(int x, int y) {
   int k = 0;
   k += (y < 0) ? x : x;
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expression are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: 'true' and 'false' expressions are equivalent
   k += (y < 0) ? x + 1 : x + 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expression are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expressions are equivalent
+  k += (y < 0) ? COND_OP_MACRO : COND_OP_MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'true' and 'false' expressions are equivalent
+
+  // Do not match for conditional operators with a macro and a const.
+  k += (y < 0) ? COND_OP_MACRO : 9;
+  // Do not match for conditional operators with expressions from different macros.
+  k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO;
   return k;
 }
+#undef COND_OP_MACRO
+#undef COND_OP_OTHER_MACRO
 
 struct MyStruct {
   int x;
 } Q;
+
 bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; }
-bool TestOperator(const MyStruct& S) {
+
+bool TestOperator(MyStruct& S) {
   if (S == Q) return false;
-  return S == S;
-  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: both side of overloaded operator are equivalent
+  if (S == S) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 }
 
 #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;
 }
 
 int TestFalsePositive(int* A, int X, float F) {
   // Produced by bison.
   X = A[(2) - (2)];
   X = A['a' - 'a'];
 
   // Testing NaN.
   if (F != F && F == F) return 1;
   return 0;
 }
 
 int TestBannedMacros() {
 #define EAGAIN 3
 #define NOT_EAGAIN 3
   if (EAGAIN == 0 | EAGAIN == 0) return 0;
   if (NOT_EAGAIN == 0 | NOT_EAGAIN == 0) return 0;
-  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: both sides of operator are equivalent
+  return 0;
 }
 
 struct MyClass {
 static const int Value = 42;
 };
 template <typename T, typename U>
 void TemplateCheck() {
   static_assert(T::Value == U::Value, "should be identical");
   static_assert(T::Value == T::Value, "should be identical");
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both side of overloaded operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of overloaded operator are equivalent
 }
 void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }
 
 int TestArithmetic(int X, int Y) {
   if (X + 1 == X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
   if (X + 1 != X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
int TestArithmetic(int X, int Y) {
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
   if (X + 1 != X - -2) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
 
   if (X + 1 == X - (~0)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
   if (X + 1 == X - (~0U)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
   if (X + 1 == X - (~0ULL)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
 
   // Should not match.
   if (X + 0.5 == X) return 1;
   if (X + 1 == Y) return 1;
   if (X + 1 == Y + 1) return 1;
   if (X + 1 == Y + 2) return 1;
 
   return 0;
 }
 
-int TestBitwise(int X) {
+int TestBitwise(int X, int Y) {
+
   if ((X & 0xFF) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
   if ((X & 0xFF) != 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
   if ((X | 0xFF) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
   if ((X | 0xFF) != 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
int TestBitwise(int X, int Y) {
   if ((0xFFLL & X) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: logical expression is always false
   if ((0xFF & X) == 0xF00ULL) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
 
   return 0;
 }
 
-int TestRelational(int X, int Y) {
+int TestLogical(int X, int Y){
+#define CONFIG 0
+  if (CONFIG && X) return 1; // OK, consts from macros are considered intentional
+#undef CONFIG
+#define CONFIG 1
+  if (CONFIG || X) return 1;
+#undef CONFIG
+
   if (X == 10 && X != 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X == 10 && (X != 10)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X == 10 && !(X == 10)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (!(X != 10) && !(X == 10)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
#undef CONFIG
   if (X == 0 && X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
   if (X != 0 && !X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
   if (X && !X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: logical expression is always false
 
   if (X && !!X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: equivalent expression on both sides of logical operator
   if (X != 0 && X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator
   if (X != 0 && !!X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator
   if (X == 0 && !X) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator
+
+  // Should not match.
+  if (X == 10 && Y == 10) return 1;
+  if (X != 10 && X != 12) return 1;
+  if (X == 10 || X == 12) return 1;
+  if (!X && !Y) return 1;
+  if (!X && Y) return 1;
+  if (!X && Y == 0) return 1;
+  if (X == 10 && Y != 10) return 1;
+}
 
+int TestRelational(int X, int Y) {
   if (X == 10 && X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X == 10 && X < 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X < 10 && X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
   if (X <= 10 && X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
int TestRelational(int X, int Y) {
   if (X < 10 || X >= 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
   if (X <= 10 || X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
   if (X <= 10 || X >= 11) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
   if (X != 7 || X != 14) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
+  if (X == 7 || X != 5) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X != 7 || X == 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always true
 
   if (X < 7 && X < 6) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X < 7 && X < 7) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X < 7 && X < 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
 
   if (X < 7 && X <= 5) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X < 7 && X <= 6) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: equivalent expression on both sides of logical operator
   if (X < 7 && X <= 7) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
   if (X < 7 && X <= 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
 
   if (X <= 7 && X < 6) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X <= 7 && X < 7) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X <= 7 && X < 8) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both side of logical operator
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator
+
+  if (X >= 7 && X > 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: equivalent expression on both sides of logical operator
+  if (X >= 7 && X > 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X >= 7 && X > 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
 
   if (X <= 7 && X <= 5) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X <= 7 && X <= 6) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X <= 7 && X <= 7) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: both sides of operator are equivalent
   if (X <= 7 && X <= 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: expression is redundant
 
   if (X == 11 && X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: expression is redundant
   if (X == 11 && X < 12) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: expression is redundant
   if (X > 10 && X == 11) return 1;
int TestRelational(int X, int Y) {
   if (X != 11 && X < 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
   if (X != 11 && X > 14) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
 
   if (X < 7 || X < 6) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
   if (X < 7 || X < 7) return 1;
-  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both side of operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
   if (X < 7 || X < 8) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
 
+  if (X > 7 || X > 6) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X > 7 || X > 7) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: both sides of operator are equivalent
+  if (X > 7 || X > 8) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: expression is redundant
+
   // Should not match.
-  if (X == 10 && Y == 10) return 1;
-  if (X != 10 && X != 12) return 1;
-  if (X == 10 || X == 12) return 1;
   if (X < 10 || X > 12) return 1;
   if (X > 10 && X < 12) return 1;
   if (X < 10 || X >= 12) return 1;
   if (X > 10 && X <= 12) return 1;
   if (X <= 10 || X > 12) return 1;
   if (X >= 10 && X < 12) return 1;
   if (X <= 10 || X >= 12) return 1;
   if (X >= 10 && X <= 12) return 1;
   if (X >= 10 && X <= 11) return 1;
   if (X >= 10 && X < 11) return 1;
   if (X > 10 && X <= 11) return 1;
   if (X > 10 && X != 11) return 1;
   if (X >= 10 && X <= 10) return 1;
   if (X <= 10 && X >= 10) return 1;
-  if (!X && !Y) return 1;
-  if (!X && Y) return 1;
-  if (!X && Y == 0) return 1;
-  if (X == 10 && Y != 10) return 1;
   if (X < 0 || X > 0) return 1;
+}
 
+int TestRelationalMacros(int X){
+#define SOME_MACRO 3
+#define SOME_MACRO_SAME_VALUE 3
+#define SOME_OTHER_MACRO 9
+  // Do not match for redundant relational macro expressions that can be
+  // considered intentional, and for some particular values, non redundant.
+
+  // Test cases for expressions with the same macro on both sides.
+  if (X < SOME_MACRO && X > SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always false
+  if (X < SOME_MACRO && X == SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always false
+  if (X < SOME_MACRO || X >= SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always true
+  if (X <= SOME_MACRO || X > SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: logical expression is always true
+  if (X != SOME_MACRO && X > SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+  if (X != SOME_MACRO && X < SOME_MACRO) return 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: expression is redundant
+
+  // Test cases for two different macros.
+  if (X < SOME_MACRO && X > SOME_OTHER_MACRO) return 1;
+  if (X != SOME_MACRO && X >= SOME_OTHER_MACRO) return 1;
+  if (X != SOME_MACRO && X != SOME_OTHER_MACRO) return 1;
+  if (X == SOME_MACRO || X == SOME_MACRO_SAME_VALUE) return 1;
+  if (X == SOME_MACRO || X <= SOME_MACRO_SAME_VALUE) return 1;
+  if (X == SOME_MACRO || X > SOME_MACRO_SAME_VALUE) return 1;
+  if (X < SOME_MACRO && X <= SOME_OTHER_MACRO) return 1;
+  if (X == SOME_MACRO && X > SOME_OTHER_MACRO) return 1;
+  if (X == SOME_MACRO && X != SOME_OTHER_MACRO) return 1;
+  if (X == SOME_MACRO && X != SOME_MACRO_SAME_VALUE) return 1;
+  if (X == SOME_MACRO_SAME_VALUE && X == SOME_MACRO ) return 1;
+
+  // Test cases for a macro and a const.
+  if (X < SOME_MACRO && X > 9) return 1;
+  if (X != SOME_MACRO && X >= 9) return 1;
+  if (X != SOME_MACRO && X != 9) return 1;
+  if (X == SOME_MACRO || X == 3) return 1;
+  if (X == SOME_MACRO || X <= 3) return 1;
+  if (X < SOME_MACRO && X <= 9) return 1;
+  if (X == SOME_MACRO && X != 9) return 1;
+  if (X == SOME_MACRO && X == 9) return 1;
+
+#undef SOME_OTHER_MACRO
+#undef SOME_MACRO_SAME_VALUE
+#undef SOME_MACRO
   return 0;
 }
 
 int TestValidExpression(int X) {
   if (X - 1 == 1 - X) return 1;
   if (2 * X == X) return 1;
   if ((X << 1) == X) return 1;
 
   return 0;
 }
 
 enum Color { Red, Yellow, Green };
-int TestRelatiopnalWithEnum(enum Color C) {
+int TestRelationalWithEnum(enum Color C) {
   if (C == Red && C == Yellow) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
   if (C == Red && C != Red) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always false
   if (C != Red || C != Yellow) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: logical expression is always true
 
   // Should not match.
   if (C == Red || C == Yellow) return 1;
   if (C != Red && C != Yellow) return 1;
 
   return 0;
 }
 
 template<class T>
 int TestRelationalTemplated(int X) {
   // This test causes a corner case with |isIntegerConstantExpr| where the type
-  // is dependant. There is an assert failing when evaluating
+  // is dependent. There is an assert failing when evaluating
   // sizeof(<incomplet-type>).
   if (sizeof(T) == 4 || sizeof(T) == 8) return 1;
 
   if (X + 0 == -X) return 1;
   if (X + 0 < X) return 1;
 
   return 0;
 }
 
 int TestWithSignedUnsigned(int X) {
   if (X + 1 == X + 1ULL) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
+
   if ((X & 0xFFU) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false
+
   if ((X & 0xFF) == 0xF00U) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
+
   if ((X & 0xFFU) == 0xF00U) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: logical expression is always false
 
   return 0;
 }
 
 int TestWithLong(int X, I64 Y) {
   if (X + 0ULL == -X) return 1;
   if (Y + 0 == -Y) return 1;
   if (Y <= 10 && X >= 10LL) return 1;
   if (Y <= 10 && X >= 10ULL) return 1;
   if (X <= 10 || X > 12LL) return 1;
   if (X <= 10 || X > 12ULL) return 1;
   if (Y <= 10 || Y > 12) return 1;
 
   return 0;
 }
 
 int TestWithMinMaxInt(int X) {
   if (X <= X + 0xFFFFFFFFU) return 1;
   if (X <= X + 0x7FFFFFFF) return 1;
   if (X <= X + 0x80000000) return 1;
+
   if (X <= 0xFFFFFFFFU && X > 0) return 1;
   if (X <= 0xFFFFFFFFU && X > 0U) return 1;
 
   if (X + 0x80000000 == X - 0x80000000) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: logical expression is always true
 
   if (X > 0x7FFFFFFF || X < ((-0x7FFFFFFF)-1)) return 1;
   if (X <= 0x7FFFFFFF && X >= ((-0x7FFFFFFF)-1)) return 1;
 
   return 0;
 }