Refactor Usenullptr matcher to avoid duplication

Previously UseNullptr matched separately implicit and explicit casts to nullptr,
now it matches casts that either are implict casts to nullptr or have an
implicit cast to nullptr within.

Also fixes PR15572 since the same macro replacement logic is applied to implicit
and explicit casts.

llvm-svn: 178494

6 files changed, 91 insertions, 96 deletions

diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
index 79b8b5fc1b4..53c6341b31c 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
@@ -69,21 +69,25 @@ llvm::StringRef GetOutermostMacroName(
 }
 }
 
-/// \brief Looks for a sequences of 0 or more explicit casts with an implicit
-/// null-to-pointer cast within.
+/// \brief Looks for implicit casts as well as sequences of 0 or more explicit
+/// casts with an implicit null-to-pointer cast within.
 ///
-/// The matcher this visitor is used with will find a top-most explicit cast
-/// (i.e. it has no explicit casts as an ancestor) where an implicit cast is
-/// nested within. However, there is no guarantee that only explicit casts
-/// exist between the found top-most explicit cast and the possibly more than
-/// one nested implicit cast. This visitor finds all cast sequences with an
-/// implicit cast to null within and creates a replacement leaving the
-/// outermost explicit cast unchanged to avoid introducing ambiguities.
+/// The matcher this visitor is used with will find a single implicit cast or a
+/// top-most explicit cast (i.e. it has no explicit casts as an ancestor) where
+/// an implicit cast is nested within. However, there is no guarantee that only
+/// explicit casts exist between the found top-most explicit cast and the
+/// possibly more than one nested implicit cast. This visitor finds all cast
+/// sequences with an implicit cast to null within and creates a replacement
+/// leaving the outermost explicit cast unchanged to avoid introducing
+/// ambiguities.
 class CastSequenceVisitor : public RecursiveASTVisitor<CastSequenceVisitor> {
 public:
   CastSequenceVisitor(tooling::Replacements &R, SourceManager &SM,
+                      const LangOptions &LangOpts,
+                      const UserMacroNames &UserNullMacros,
                       unsigned &AcceptedChanges)
-      : Replace(R), SM(SM), AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {}
+      : Replace(R), SM(SM), LangOpts(LangOpts), UserNullMacros(UserNullMacros),
+        AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {}
 
   // Only VisitStmt is overridden as we shouldn't find other base AST types
   // within a cast expression.
@@ -94,8 +98,14 @@ public:
       ResetFirstSubExpr();
       return true;
     } else if (!FirstSubExpr) {
-      // Get the subexpression of the outermost explicit cast
-      FirstSubExpr = C->getSubExpr();
+      // Keep parentheses for implicit casts to avoid cases where an implicit
+      // cast within a parentheses expression is right next to a return
+      // statement otherwise get the subexpression of the outermost explicit
+      // cast.
+      if (C->getStmtClass() == Stmt::ImplicitCastExprClass)
+        FirstSubExpr = C->IgnoreParenImpCasts();
+      else
+        FirstSubExpr = C->getSubExpr();
     }
 
     if (C->getCastKind() == CK_NullToPointer ||
@@ -104,9 +114,28 @@ public:
       SourceLocation StartLoc = FirstSubExpr->getLocStart();
       SourceLocation EndLoc = FirstSubExpr->getLocEnd();
 
-      // If the start/end location is a macro, get the expansion location.
-      StartLoc = SM.getFileLoc(StartLoc);
-      EndLoc = SM.getFileLoc(EndLoc);
+      // If the start/end location is a macro argument expansion, get the
+      // expansion location. If its a macro body expansion, check to see if its
+      // coming from a macro called NULL.
+      if (SM.isMacroArgExpansion(StartLoc) && SM.isMacroArgExpansion(EndLoc)) {
+        StartLoc = SM.getFileLoc(StartLoc);
+        EndLoc = SM.getFileLoc(EndLoc);
+      } else if (SM.isMacroBodyExpansion(StartLoc) &&
+                 SM.isMacroBodyExpansion(EndLoc)) {
+        llvm::StringRef OutermostMacroName =
+            GetOutermostMacroName(StartLoc, SM, LangOpts);
+
+        // Check to see if the user wants to replace the macro being expanded.
+        bool ReplaceNullMacro =
+            std::find(UserNullMacros.begin(), UserNullMacros.end(),
+                      OutermostMacroName) != UserNullMacros.end();
+
+        if (!ReplaceNullMacro)
+          return false;
+
+        StartLoc = SM.getFileLoc(StartLoc);
+        EndLoc = SM.getFileLoc(EndLoc);
+      }
 
       AcceptedChanges +=
           ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0;
@@ -123,6 +152,8 @@ private:
 private:
   tooling::Replacements &Replace;
   SourceManager &SM;
+  const LangOptions &LangOpts;
+  const UserMacroNames &UserNullMacros;
   unsigned &AcceptedChanges;
   Expr *FirstSubExpr;
 };
@@ -141,44 +172,11 @@ void NullptrFixer::run(const ast_matchers::MatchFinder::MatchResult &Result) {
   SourceManager &SM = *Result.SourceManager;
 
   const CastExpr *NullCast = Result.Nodes.getNodeAs<CastExpr>(CastSequence);
-  if (NullCast) {
-    // Given an explicit cast with an implicit null-to-pointer cast within
-    // use CastSequenceVisitor to identify sequences of explicit casts that can
-    // be converted into 'nullptr'.
-    CastSequenceVisitor Visitor(Replace, SM, AcceptedChanges);
-    Visitor.TraverseStmt(const_cast<CastExpr *>(NullCast));
-  }
-
-  const CastExpr *Cast = Result.Nodes.getNodeAs<CastExpr>(ImplicitCastNode);
-  if (Cast) {
-    const Expr *E = Cast->IgnoreParenImpCasts();
-    SourceLocation StartLoc = E->getLocStart();
-    SourceLocation EndLoc = E->getLocEnd();
-
-    // If the start/end location is a macro argument expansion, get the
-    // expansion location. If its a macro body expansion, check to see if its
-    // coming from a macro called NULL.
-    if (SM.isMacroArgExpansion(StartLoc) && SM.isMacroArgExpansion(EndLoc)) {
-      StartLoc = SM.getFileLoc(StartLoc);
-      EndLoc = SM.getFileLoc(EndLoc);
-    } else if (SM.isMacroBodyExpansion(StartLoc) &&
-               SM.isMacroBodyExpansion(EndLoc)) {
-      llvm::StringRef OutermostMacroName =
-          GetOutermostMacroName(StartLoc, SM, Result.Context->getLangOpts());
-
-      // Check to see if the user wants to replace the macro being expanded.
-      bool ReplaceNullMacro =
-          std::find(UserNullMacros.begin(), UserNullMacros.end(),
-                    OutermostMacroName) != UserNullMacros.end();
-
-      if (!ReplaceNullMacro)
-        return;
-
-      StartLoc = SM.getFileLoc(StartLoc);
-      EndLoc = SM.getFileLoc(EndLoc);
-    }
-
-    AcceptedChanges +=
-        ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0;
-  }
+  assert(NullCast && "Bad Callback. No node provided");
+  // Given an implicit null-ptr cast or an explicit cast with an implicit
+  // null-to-pointer cast within use CastSequenceVisitor to identify sequences
+  // of explicit casts that can be converted into 'nullptr'.
+  CastSequenceVisitor Visitor(Replace, SM, Result.Context->getLangOpts(),
+     UserNullMacros, AcceptedChanges);
+  Visitor.TraverseStmt(const_cast<CastExpr *>(NullCast));
 }
diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h
index a79cb301543..9835a5375e2 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.h
@@ -19,6 +19,9 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Tooling/Refactoring.h"
 
+// The type for user-defined macro names that behave like NULL
+typedef llvm::SmallVector<llvm::StringRef, 1> UserMacroNames;
+
 /// \brief The callback to be used for nullptr migration matchers.
 ///
 class NullptrFixer : public clang::ast_matchers::MatchFinder::MatchCallback {
@@ -33,7 +36,7 @@ public:
 private:
   clang::tooling::Replacements &Replace;
   unsigned &AcceptedChanges;
-  llvm::SmallVector<llvm::StringRef, 1> UserNullMacros;
+  UserMacroNames UserNullMacros;
 };
 
 #endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_NULLPTR_ACTIONS_H
diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp
index 86e00c189d5..e4baf05432f 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.cpp
@@ -18,7 +18,6 @@
 using namespace clang::ast_matchers;
 using namespace clang;
 
-const char *ImplicitCastNode = "cast";
 const char *CastSequence = "sequence";
 
 namespace clang {
@@ -47,30 +46,22 @@ AST_MATCHER(Type, sugaredNullptrType) {
 } // end namespace ast_matchers
 } // end namespace clang
 
-StatementMatcher makeImplicitCastMatcher() {
-  return implicitCastExpr(
-           isNullToPointer(),
-           unless(hasAncestor(explicitCastExpr())),
-           unless(
-             hasSourceExpression(
-               hasType(sugaredNullptrType())
-             )
-           )
-         ).bind(ImplicitCastNode);
-}
-
 StatementMatcher makeCastSequenceMatcher() {
-  return explicitCastExpr(
+  StatementMatcher ImplicitCastToNull =
+    implicitCastExpr(
+      isNullToPointer(),
+      unless(
+        hasSourceExpression(
+          hasType(sugaredNullptrType())
+        )
+      )
+    );
+
+  return castExpr(
            unless(hasAncestor(explicitCastExpr())),
-           hasDescendant(
-             implicitCastExpr(
-               isNullToPointer(),
-               unless(
-                 hasSourceExpression(
-                   hasType(sugaredNullptrType())
-                 )
-               )
-             )
+           anyOf(
+             hasDescendant(ImplicitCastToNull),
+             ImplicitCastToNull
            )
          ).bind(CastSequence);
 }
diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h
index e6d23175480..bb10e54bd36 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrMatchers.h
@@ -18,21 +18,13 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 
 // Names to bind with matched expressions.
-extern const char *ImplicitCastNode;
 extern const char *CastSequence;
 
-/// \brief Create a matcher to find the implicit casts clang inserts
-/// when writing null to a pointer.
-///
-/// However, don't match those implicit casts that have explicit casts as
-/// an ancestor. Explicit casts are handled by makeCastSequenceMatcher().
-clang::ast_matchers::StatementMatcher makeImplicitCastMatcher();
-
-/// \brief Create a matcher that finds the head of a sequence of nested explicit
-/// casts that have an implicit cast to null within.
-///
-/// This matcher is necessary so that an entire sequence of explicit casts can
-/// be replaced instead of just the inner-most implicit cast.
+/// \brief Create a matcher that finds implicit casts as well as the head of a
+/// sequence of zero or more nested explicit casts that have an implicit cast
+/// to null within.
+/// Finding sequences of explict casts is necessary so that an entire sequence
+/// can be replaced instead of just the inner-most implicit cast.
 clang::ast_matchers::StatementMatcher makeCastSequenceMatcher();
 
 #endif // LLVM_TOOLS_CLANG_TOOLS_EXTRA_CPP11_MIGRATE_USE_NULLPTR_MATCHERS_H
diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp
index fe90cfcccb4..573604a9c53 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/UseNullptr.cpp
@@ -45,7 +45,6 @@ int UseNullptrTransform::apply(const FileContentsByPath &InputStates,
                      AcceptedChanges,
                      MaxRisk);
 
-  Finder.addMatcher(makeImplicitCastMatcher(), &Fixer);
   Finder.addMatcher(makeCastSequenceMatcher(), &Fixer);
 
   if (int result = UseNullptrTool.run(newFrontendActionFactory(&Finder))) {
diff --git a/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp b/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp
index 28de7458c60..8e19e4ab6dc 100644
--- a/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp
+++ b/clang-tools-extra/test/cpp11-migrate/UseNullptr/macros.cpp
@@ -39,8 +39,22 @@ void test_macro_expansion1() {
 #undef MACRO_EXPANSION_HAS_NULL
 }
 
+// Test macro expansion with cast sequence, PR15572
 void test_macro_expansion2() {
 #define MACRO_EXPANSION_HAS_NULL \
+  dummy((int*)0); \
+  side_effect();
+  // CHECK: dummy((int*)0); \
+  // CHECK-NEXT: side_effect();
+
+  MACRO_EXPANSION_HAS_NULL;
+  // CHECK: MACRO_EXPANSION_HAS_NULL;
+
+#undef MACRO_EXPANSION_HAS_NULL
+}
+
+void test_macro_expansion3() {
+#define MACRO_EXPANSION_HAS_NULL \
   dummy(NULL); \
   side_effect();
   // CHECK: dummy(NULL); \
@@ -57,7 +71,7 @@ void test_macro_expansion2() {
 #undef MACRO_EXPANSION_HAS_NULL
 }
 
-void test_macro_expansion3() {
+void test_macro_expansion4() {
 #define MY_NULL NULL
   int *p = MY_NULL;
   // CHECK: int *p = MY_NULL;
@@ -91,5 +105,3 @@ void test_function_like_macro2() {
   // CHECK: my_macro(p != nullptr);
 #undef my_macro
 }
-
-