[clang-tidy] Address false positives in misc-redundant-expression checker

Address situations when misc-redundant-expression checker provides excessive
diagnostics for situations with different macros having the same value.
In perticular situations described in the initial report of
#118885 are addressed.
The situations which are popped inside discussion like
if (A + B == B + A) for macros are not properly addressed by this patch.
But I still believe even in the current state the patch can be useful.

Author: Vladislav Aranov

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

@@ -139,10 +139,8 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
     return cast<BinaryOperator>(Left)->getOpcode() ==
            cast<BinaryOperator>(Right)->getOpcode();
   case Stmt::UnaryExprOrTypeTraitExprClass:
-    const auto *LeftUnaryExpr =
-        cast<UnaryExprOrTypeTraitExpr>(Left);
-    const auto *RightUnaryExpr =
-        cast<UnaryExprOrTypeTraitExpr>(Right);
+    const auto *LeftUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Left);
+    const auto *RightUnaryExpr = cast<UnaryExprOrTypeTraitExpr>(Right);
     if (LeftUnaryExpr->isArgumentType() && RightUnaryExpr->isArgumentType())
       return LeftUnaryExpr->getKind() == RightUnaryExpr->getKind() &&
              LeftUnaryExpr->getArgumentType() ==
@@ -699,7 +697,8 @@ static bool retrieveRelationalIntegerConstantExpr(
 
     Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
     OperandExpr = OverloadedOperatorExpr;
-    Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
+    Opcode = BinaryOperator::getOverloadedOpcode(
+        OverloadedOperatorExpr->getOperator());
 
     if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
       return false;
@@ -728,7 +727,8 @@ static bool retrieveRelationalIntegerConstantExpr(
 }
 
 // Checks for expressions like (X == 4) && (Y != 9)
-static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+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());
 
@@ -747,6 +747,28 @@ static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const A
   return false;
 }
 
+static bool areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+    const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+  if (areSidesBinaryConstExpressions(BinOp, AstCtx))
+    return true;
+
+  const Expr *Lhs = BinOp->getLHS();
+  const Expr *Rhs = BinOp->getRHS();
+
+  if (!Lhs || !Rhs)
+    return false;
+
+  auto IsDefineExpr = [AstCtx](const Expr *E) {
+    const SourceRange Lsr = E->getSourceRange();
+    if (!Lsr.getBegin().isMacroID() || E->isValueDependent() ||
+        !E->isIntegerConstantExpr(*AstCtx))
+      return false;
+    return true;
+  };
+
+  return IsDefineExpr(Lhs) || IsDefineExpr(Rhs);
+}
+
 // Retrieves integer constant subexpressions from binary operator expressions
 // that have two equivalent sides.
 // E.g.: from (X == 5) && (X == 5) retrieves 5 and 5.
@@ -785,7 +807,7 @@ static bool retrieveConstExprFromBothSides(const BinaryOperator *&BinOp,
 }
 
 static bool isSameRawIdentifierToken(const Token &T1, const Token &T2,
-                        const SourceManager &SM) {
+                                     const SourceManager &SM) {
   if (T1.getKind() != T2.getKind())
     return false;
   if (T1.isNot(tok::raw_identifier))
@@ -808,8 +830,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
                                         const ASTContext *AstCtx) {
   if (!LhsExpr || !RhsExpr)
     return false;
-  SourceRange Lsr = LhsExpr->getSourceRange();
-  SourceRange Rsr = RhsExpr->getSourceRange();
+  const SourceRange Lsr = LhsExpr->getSourceRange();
+  const SourceRange Rsr = RhsExpr->getSourceRange();
   if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
     return false;
 
@@ -847,11 +869,104 @@ static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
   if (!LhsExpr || !RhsExpr)
     return false;
 
-  SourceLocation LhsLoc = LhsExpr->getExprLoc();
-  SourceLocation RhsLoc = RhsExpr->getExprLoc();
+  const SourceLocation LhsLoc = LhsExpr->getExprLoc();
+  const SourceLocation RhsLoc = RhsExpr->getExprLoc();
 
   return LhsLoc.isMacroID() != RhsLoc.isMacroID();
 }
+
+static bool areStringsSameIgnoreSpaces(const llvm::StringRef *Left,
+                                       const llvm::StringRef *Right) {
+  if (Left == Right)
+    return true;
+  if (Left->compare(*Right) == 0) {
+    return true;
+  }
+  // Do running index comparison
+  size_t LIdx = 0;
+  size_t RIdx = 0;
+  const char *LData = Left->data();
+  const char *RData = Right->data();
+  while (LIdx < Left->size() && RIdx < Right->size()) {
+
+    // Skip spaces and tabs from both strings
+    while (LIdx < Left->size() && (LData[LIdx] == ' ' || LData[LIdx] == '\t')) {
+      LIdx += 1;
+    }
+    while (RIdx < Right->size() &&
+           (RData[RIdx] == ' ' || RData[RIdx] == '\t')) {
+      RIdx += 1;
+    }
+
+    // If all not tabs and spaces are the same then return true
+    if (LIdx >= Left->size() && RIdx >= Right->size())
+      return true;
+
+    // If any characters differs then return false
+    else if (LData[LIdx] != RData[RIdx])
+      return false;
+
+    // If current char is the same ==> continue search
+    else {
+      LIdx += 1;
+      RIdx += 1;
+    }
+  }
+  // Shortest string is processed: issue a verdict
+  return LIdx >= Left->size() && RIdx >= Right->size();
+}
+
+static bool areExprsSameMacroOrLiteral(const BinaryOperator *BinOp,
+                                       const ASTContext *Context) {
+
+  if (!BinOp)
+    return false;
+
+  const Expr *Lhs = BinOp->getLHS();
+  const Expr *Rhs = BinOp->getRHS();
+  const SourceManager &SM = Context->getSourceManager();
+
+  const SourceRange Lsr = Lhs->getSourceRange();
+  const SourceRange Rsr = Rhs->getSourceRange();
+  if (Lsr.getBegin().isMacroID()) {
+    // Left is macro so right macro too
+    if (Rsr.getBegin().isMacroID()) {
+      // Both sides are macros so they are same macro or literal
+      const llvm::StringRef L = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(Lsr), SM, LangOptions(), 0);
+      const llvm::StringRef R = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(Rsr), SM, LangOptions(), 0);
+      return areStringsSameIgnoreSpaces(&L, &R);
+    }
+    // Left is macro but right is not so they are not same macro or literal
+    return false;
+  } else {
+    const auto *Lil = dyn_cast<IntegerLiteral>(Lhs);
+    const auto *Ril = dyn_cast<IntegerLiteral>(Rhs);
+    if (Lil && Ril) {
+      return Lil->getValue() == Ril->getValue();
+    }
+
+    const auto *LStrl = dyn_cast<StringLiteral>(Lhs);
+    const auto *RStrl = dyn_cast<StringLiteral>(Rhs);
+    if (Lil && Ril) {
+      const llvm::StringRef L = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(LStrl->getBeginLoc()), SM,
+          LangOptions(), 0);
+      const llvm::StringRef R = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(RStrl->getBeginLoc()), SM,
+          LangOptions(), 0);
+      return L.compare(R) == 0;
+    }
+
+    const auto *Lbl = dyn_cast<CXXBoolLiteralExpr>(Lhs);
+    const auto *Rbl = dyn_cast<CXXBoolLiteralExpr>(Rhs);
+    if (Lbl && Rbl) {
+      return Lbl->getValue() == Rbl->getValue();
+    }
+  }
+  return false;
+}
 } // namespace
 
 void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
@@ -1089,7 +1204,6 @@ static bool exprEvaluatesToSymbolic(BinaryOperatorKind Opcode, APSInt Value) {
          ((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
 }
 
-
 void RedundantExpressionCheck::checkBitwiseExpr(
     const MatchFinder::MatchResult &Result) {
   if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
@@ -1134,8 +1248,8 @@ void RedundantExpressionCheck::checkBitwiseExpr(
                                      ConstExpr))
       return;
 
-    if((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
-        return;
+    if ((Value != 0 && ~Value != 0) || Sym->getExprLoc().isMacroID())
+      return;
 
     SourceLocation Loc = IneffectiveOperator->getOperatorLoc();
 
@@ -1276,19 +1390,23 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
       return;
     }
 
-    if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+    if (areSidesBinaryConstExpressionsOrDefinesOrIntegerConstant(
+            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;
+      if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
+        if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
+                                            LhsConst, RhsConst, Result.Context))
+          return;
+
+        if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
+            areExprsMacroAndNonMacro(LhsConst, RhsConst))
+          return;
+      } else {
+        if (!areExprsSameMacroOrLiteral(BinOp, Result.Context))
+          return;
+      }
     }
-
     diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
   }