[PATCH] Fixed UseNullptr causes ambiguity

[Ariel Bernal]

  - Fixed CastSequenceVisitor doxygen comment
  - Refactored visitor.

Hi revane, tareqsiraj,

http://llvm-reviews.chandlerc.com/D507

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D507?vs=1215&id=1261#toc

Files:
  cpp11-migrate/UseNullptr/NullptrActions.cpp
  test/cpp11-migrate/UseNullptr/basic.cpp

Index: cpp11-migrate/UseNullptr/NullptrActions.cpp
===================================================================
--- cpp11-migrate/UseNullptr/NullptrActions.cpp
+++ cpp11-migrate/UseNullptr/NullptrActions.cpp
@@ -49,51 +49,54 @@
 /// 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.
   bool VisitStmt(Stmt *S) {
     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);
       EndLoc = SM.getFileLoc(EndLoc);
 
       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) {
Index: test/cpp11-migrate/UseNullptr/basic.cpp
===================================================================
--- test/cpp11-migrate/UseNullptr/basic.cpp
+++ test/cpp11-migrate/UseNullptr/basic.cpp
@@ -69,16 +69,16 @@
 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 @@
 
 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;
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D507.2.patch
Type: text/x-patch
Size: 5102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130312/a3f7fe50/attachment.bin>