Don't include outer-most explicit cast in nullptr replacement

The outer-most explicit cast is now left alone by the Use-Nullptr transform to
maintain the type of the expression and avoid introducing ambiguities.

Fixes PR15395.

Author: Ariel J Bernal <ariel.j.bernal@intel.com>
llvm-svn: 177179

2 files changed, 61 insertions, 17 deletions

diff --git a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
index d0f2e0f0713..7fcd16e9e5e 100644
--- a/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
+++ b/clang-tools-extra/cpp11-migrate/UseNullptr/NullptrActions.cpp
@@ -49,12 +49,13 @@ bool ReplaceWithNullptr(tooling::Replacements &Replace, SourceManager &SM,
 /// 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.
+/// 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,
                       unsigned &AcceptedChanges)
-      : Replace(R), SM(SM), AcceptedChanges(AcceptedChanges), FirstCast(0) {}
+      : Replace(R), SM(SM), AcceptedChanges(AcceptedChanges), FirstSubExpr(0) {}
 
   // Only VisitStmt is overridden as we shouldn't find other base AST types
   // within a cast expression.
@@ -62,16 +63,18 @@ public:
     CastExpr *C = dyn_cast<CastExpr>(S);
 
     if (!C) {
-      ResetFirstCast();
+      ResetFirstSubExpr();
       return true;
-    } else if (!FirstCast) {
-      FirstCast = C;
+    } else if (!FirstSubExpr) {
+      // Get the subexpression of the outermost explicit cast
+      FirstSubExpr = C->getSubExpr();
     }
 
     if (C->getCastKind() == CK_NullToPointer ||
         C->getCastKind() == CK_NullToMemberPointer) {
-      SourceLocation StartLoc = FirstCast->getLocStart();
-      SourceLocation EndLoc = FirstCast->getLocEnd();
+
+      SourceLocation StartLoc = FirstSubExpr->getLocStart();
+      SourceLocation EndLoc = FirstSubExpr->getLocEnd();
 
       // If the start/end location is a macro, get the expansion location.
       StartLoc = SM.getFileLoc(StartLoc);
@@ -80,20 +83,20 @@ public:
       AcceptedChanges +=
           ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0;
 
-      ResetFirstCast();
+      ResetFirstSubExpr();
     }
 
     return true;
   }
 
 private:
-  void ResetFirstCast() { FirstCast = 0; }
+  void ResetFirstSubExpr() { FirstSubExpr = 0; }
 
 private:
   tooling::Replacements &Replace;
   SourceManager &SM;
   unsigned &AcceptedChanges;
-  CastExpr *FirstCast;
+  Expr *FirstSubExpr;
 };
 
 void NullptrFixer::run(const ast_matchers::MatchFinder::MatchResult &Result) {
diff --git a/clang-tools-extra/test/cpp11-migrate/UseNullptr/basic.cpp b/clang-tools-extra/test/cpp11-migrate/UseNullptr/basic.cpp
index de69a1fd3d6..46cc9cf83dc 100644
--- a/clang-tools-extra/test/cpp11-migrate/UseNullptr/basic.cpp
+++ b/clang-tools-extra/test/cpp11-migrate/UseNullptr/basic.cpp
@@ -69,16 +69,16 @@ template <typename T>
 struct Bar {
   Bar(T *p) : m_p(p) {
     m_p = static_cast<T*>(NULL);
-    // CHECK: m_p = nullptr;
+    // CHECK: m_p = static_cast<T*>(nullptr);
 
     m_p = static_cast<T*>(reinterpret_cast<int*>((void*)NULL));
-    // CHECK: m_p = nullptr;
+    // CHECK: m_p = static_cast<T*>(nullptr);
 
     m_p = static_cast<T*>(p ? p : static_cast<void*>(g_null));
-    // CHECK: m_p = static_cast<T*>(p ? p : nullptr);
+    // CHECK: m_p = static_cast<T*>(p ? p : static_cast<void*>(nullptr));
 
     T *p2 = static_cast<T*>(reinterpret_cast<int*>((void*)NULL));
-    // CHECK: T *p2 = nullptr;
+    // CHECK: T *p2 = static_cast<T*>(nullptr);
 
     m_p = NULL;
     // CHECK: m_p = nullptr;
@@ -223,15 +223,56 @@ int *test_nested_parentheses_expression() {
 
 void *test_parentheses_explicit_cast() {
   return(static_cast<void*>(0));
-  // CHECK: return(nullptr);
+  // CHECK: return(static_cast<void*>(nullptr));
 }
 
 void *test_parentheses_explicit_cast_sequence1() {
   return(static_cast<void*>(static_cast<int*>((void*)NULL)));
-  // CHECK: return(nullptr);
+  // CHECK: return(static_cast<void*>(nullptr));
 }
 
 void *test_parentheses_explicit_cast_sequence2() {
   return(static_cast<void*>(reinterpret_cast<int*>((float*)int(0.f))));
-  // CHECK: return(nullptr);
+  // CHECK: return(static_cast<void*>(nullptr));
+}
+
+// Test explicit cast expressions resulting in nullptr
+struct Bam {
+  Bam(int *a) {}
+  Bam(float *a) {}
+  Bam operator=(int *a) { return Bam(a); }
+  Bam operator=(float *a) { return Bam(a); }
+};
+
+void ambiguous_function(int *a) {}
+void ambiguous_function(float *a) {}
+
+void test_explicit_cast_ambiguous1() {
+  ambiguous_function((int*)0);
+  // CHECK: ambiguous_function((int*)nullptr);
+}
+
+void test_explicit_cast_ambiguous2() {
+  ambiguous_function((int*)(0));
+  // CHECK: ambiguous_function((int*)nullptr);
+}
+
+void test_explicit_cast_ambiguous3() {
+  ambiguous_function(static_cast<int*>(reinterpret_cast<int*>((float*)0)));
+  // CHECK: ambiguous_function(static_cast<int*>(nullptr));
+}
+
+Bam test_explicit_cast_ambiguous4() {
+  return(((int*)(0)));
+  // CHECK: return(((int*)nullptr));
+}
+
+void test_explicit_cast_ambiguous5() {
+  // Test for ambiguous overloaded constructors
+  Bam k((int*)(0));
+  // CHECK: Bam k((int*)nullptr);
+
+  // Test for ambiguous overloaded operators
+  k = (int*)0;
+  // CHECK: k = (int*)nullptr;
 }