[PATCH] Don't replace macro usage if macro body has NULL

[Tareq A. Siraj]

Hi klimek, revane,

In case of macro body expansion, check to see if the macro is named NULL
and don't replace inside the macro body. This fixes the case when NULL
appears inside the macro body and the transform replaces the usage of
the macro with nullptr. This is an easy fix for the problem for now and
we should analyze the macro body to see if it expands to only
NullToPointer in the future for a more robust solution that takes care
of user defined macros that behaves like NULL.

Other changes:
 - Moved complex macro tests to macros.cpp
 - Added new test cases.
 - Added checks to make sure that the macro bodies are not modified by
   the tool.

Fixes: PR15396

Signed-off-by: Tareq A. Siraj <tareq.a.siraj at intel.com>

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

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

Index: cpp11-migrate/UseNullptr/NullptrActions.cpp
===================================================================
--- cpp11-migrate/UseNullptr/NullptrActions.cpp
+++ cpp11-migrate/UseNullptr/NullptrActions.cpp
@@ -19,14 +19,15 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
 using namespace clang;
 
 namespace {
 
-/// \brief Replaces the provided range with the text "nullptr", but only if 
+/// \brief Replaces the provided range with the text "nullptr", but only if
 /// the start and end location are both in main file.
 /// Returns true if and only if a replacement was made.
 bool ReplaceWithNullptr(tooling::Replacements &Replace, SourceManager &SM,
@@ -114,13 +115,31 @@
   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, 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 ImmediateMacroName = clang::Lexer::getImmediateMacroName(
+          StartLoc, SM, Result.Context->getLangOpts());
+      if ("NULL" != ImmediateMacroName)
+        return;
+
+      SourceLocation MacroCallerStartLoc =
+          SM.getImmediateMacroCallerLoc(StartLoc);
+      SourceLocation MacroCallerEndLoc = SM.getImmediateMacroCallerLoc(EndLoc);
+
+      if (!MacroCallerStartLoc.isMacroID() && !MacroCallerEndLoc.isMacroID()) {
+        StartLoc = SM.getFileLoc(StartLoc);
+        EndLoc = SM.getFileLoc(EndLoc);
+      }
+    }
 
     AcceptedChanges +=
         ReplaceWithNullptr(Replace, SM, StartLoc, EndLoc) ? 1 : 0;
Index: test/cpp11-migrate/UseNullptr/basic.cpp
===================================================================
--- test/cpp11-migrate/UseNullptr/basic.cpp
+++ test/cpp11-migrate/UseNullptr/basic.cpp
@@ -8,6 +8,7 @@
 
 const unsigned int g_null = 0;
 #define NULL 0
+// CHECK: define NULL 0
 
 void test_assignment() {
   int *p1 = 0;
@@ -179,32 +180,6 @@
   // CHECK: return g_null;
 }
 
-// Test function-like macros where the parameter to the macro (expression)
-// results in a nullptr.
-void __dummy_assert_fail() {}
-
-void test_function_like_macro1() {
-  // This tests that the CastSequenceVisitor is working properly.
-#define my_assert(expr) \
-  ((expr) ? static_cast<void>(expr) : __dummy_assert_fail())
-
-  int *p;
-  my_assert(p != 0);
-  // CHECK: my_assert(p != nullptr);
-#undef my_assert
-}
-
-void test_function_like_macro2() {
-  // This tests that the implicit cast is working properly.
-#define my_macro(expr) \
-  (expr)
-
-  int *p;
-  my_macro(p != 0);
-  // CHECK: my_macro(p != nullptr);
-#undef my_macro
-}
-
 // Test parentheses expressions resulting in a nullptr.
 int *test_parentheses_expression1() {
   return(0);
Index: test/cpp11-migrate/UseNullptr/macros.cpp
===================================================================
--- /dev/null
+++ test/cpp11-migrate/UseNullptr/macros.cpp
@@ -0,0 +1,66 @@
+// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
+// RUN: cpp11-migrate -use-nullptr %t.cpp -- -I %S
+// RUN: FileCheck -input-file=%t.cpp %s
+
+#define NULL 0
+// CHECK: #define NULL 0
+
+void dummy(int*) {}
+void side_effect() {}
+
+#define MACRO_EXPANSION_HAS_NULL \
+  void foo() { \
+    dummy(0); \
+    dummy(NULL); \
+    side_effect(); \
+  }
+  // CHECK: void foo() { \
+  // CHECK-NEXT:   dummy(0); \
+  // CHECK-NEXT:   dummy(NULL); \
+
+MACRO_EXPANSION_HAS_NULL;
+//CHECK: MACRO_EXPANSION_HAS_NULL;
+#undef MACRO_EXPANSION_HAS_NULL
+
+
+void test_macro_expansion() {
+#define MACRO_EXPANSION_HAS_NULL \
+  dummy(NULL); \
+  side_effect();
+  // CHECK: dummy(NULL); \
+  // CHECK-NEXT: side_effect();
+
+  MACRO_EXPANSION_HAS_NULL;
+  // CHECK: MACRO_EXPANSION_HAS_NULL;
+
+#undef MACRO_EXPANSION_HAS_NULL
+}
+
+// Test function-like macros where the parameter to the macro (expression)
+// results in a nullptr.
+void __dummy_assert_fail() {}
+
+void test_function_like_macro1() {
+  // This tests that the CastSequenceVisitor is working properly.
+#define my_assert(expr) \
+  ((expr) ? static_cast<void>(expr) : __dummy_assert_fail())
+  // CHECK: ((expr) ? static_cast<void>(expr) : __dummy_assert_fail())
+
+  int *p;
+  my_assert(p != 0);
+  // CHECK: my_assert(p != nullptr);
+#undef my_assert
+}
+
+void test_function_like_macro2() {
+  // This tests that the implicit cast is working properly.
+#define my_macro(expr) \
+  (expr)
+
+  int *p;
+  my_macro(p != 0);
+  // CHECK: my_macro(p != nullptr);
+#undef my_macro
+}
+
+
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D549.1.patch
Type: text/x-patch
Size: 5215 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130318/a5dda34f/attachment.bin>