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

1 files changed, 53 insertions, 55 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));
 }