[PATCH] D55125: Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp created this revision.
dkrupp added reviewers: alexfh, aaron.ballman, gamesh411.
dkrupp added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, Szelethus, rnkovacs.

Do not warn for redundant conditional expressions
when the true and false branches are expanded from
different macros even when they are
defined by one another.

We used to get a false warning in the following case:
define M1 <https://reviews.llvm.org/M1> 1
#define M2 M1 <https://reviews.llvm.org/M1>
int test(int cond){
 return cond ? M1 <https://reviews.llvm.org/M1> : M2;//false warning: 'true' and 'false' expressions are equivalent
}

The problem was that the Lexer::getImmediateMacroName() call was used for comparing macros, which returned "M1 <https://reviews.llvm.org/M1>" for the expansion of M1 <https://reviews.llvm.org/M1> and M2.
Now we are comparing macros from token to token.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55125

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  test/clang-tidy/misc-redundant-expression.cpp


Index: test/clang-tidy/misc-redundant-expression.cpp
===================================================================
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -106,6 +106,7 @@
 
 #define COND_OP_MACRO 9
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {
   int k = 0;
   k += (y < 0) ? x : x;
@@ -114,11 +115,15 @@
   // 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
+  k += (y < 0) ? COND_OP_MACRO + COND_OP_OTHER_MACRO : COND_OP_MACRO + COND_OP_OTHER_MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: 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;
+  // Do not match for conditional operators when a macro is defined to another macro
+  k += (y < 0) ? COND_OP_MACRO : COND_OP_THIRD_MACRO;
   return k;
 }
 #undef COND_OP_MACRO
Index: clang-tidy/misc/RedundantExpressionCheck.cpp
===================================================================
--- clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -598,23 +598,55 @@
   return true;
 }
 
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM){
+  if (T1.getLength()!=T2.getLength())
+    return false;
+  return strncmp(SM.getCharacterData(T1.getLocation()),SM.getCharacterData(T2.getLocation()),T1.getLength())==0;
+}
+
+//Returns true if both LhsEpxr and RhsExpr are
+//macro expressions and they are expanded
+//from different macros.
 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())
+  SourceRange Lsr = LhsExpr->getSourceRange();
+  SourceRange Rsr = RhsExpr->getSourceRange();
+  if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
     return false;
-
   const SourceManager &SM = AstCtx->getSourceManager();
   const LangOptions &LO = AstCtx->getLangOpts();
 
-  return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) ==
-          Lexer::getImmediateMacroName(RhsLoc, SM, LO));
+  std::pair<FileID, unsigned> LsrLocInfo =
+      SM.getDecomposedLoc(SM.getExpansionLoc(Lsr.getBegin()));
+  std::pair<FileID, unsigned> RsrLocInfo =
+      SM.getDecomposedLoc(SM.getExpansionLoc(Rsr.getBegin()));
+  const llvm::MemoryBuffer *MB = SM.getBuffer(LsrLocInfo.first);
+
+  const char *LTokenPos = MB->getBufferStart() + LsrLocInfo.second;
+  const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second;
+  clang::Lexer LRawLex(SM.getLocForStartOfFile(LsrLocInfo.first), LO,
+                       MB->getBufferStart(), LTokenPos, MB->getBufferEnd());
+  clang::Lexer RRawLex(SM.getLocForStartOfFile(RsrLocInfo.first), LO,
+                       MB->getBufferStart(), RTokenPos, MB->getBufferEnd());
+
+  clang::Token LTok, RTok;
+  SourceLocation LLoc, RLoc;
+  int LRem, RRem;
+  do {//we compare the expressions token-by-token
+    LRawLex.LexFromRawLexer(LTok);
+    RRawLex.LexFromRawLexer(RTok);
+    LLoc = LTok.getLocation();
+    RLoc = RTok.getLocation();
+    LRem = SM.getCharacterData(SM.getExpansionLoc(Lsr.getEnd())) -
+           SM.getCharacterData(LLoc);//remaining characters until the end of expr
+    RRem = SM.getCharacterData(SM.getExpansionLoc(Rsr.getEnd())) -
+           SM.getCharacterData(RLoc);
+  } while (!LTok.is(clang::tok::eof) && !RTok.is(clang::tok::eof) &&
+           compareToks(LTok, RTok, SM) && LRem > 0 && RRem > 0);
+  return !((LRem == 0 && RRem == 0) && compareToks(LTok, RTok, SM));
 }
 
 static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D55125.176124.patch
Type: text/x-patch
Size: 4243 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181130/3743df58/attachment-0001.bin>