[change-namespace] get newlines around moved namespace right.

Summary: Previously, a `\n` might be left in the old namespace and thus not copied to the new namespace, which is bad.

Reviewers: hokein

Subscribers: alexshap, cfe-commits

Differential Revision: https://reviews.llvm.org/D28282

llvm-svn: 290966

2 files changed, 54 insertions, 18 deletions

diff --git a/clang-tools-extra/change-namespace/ChangeNamespace.cpp b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
index 7452b7d1f02..b2f31a9bef3 100644
--- a/clang-tools-extra/change-namespace/ChangeNamespace.cpp
+++ b/clang-tools-extra/change-namespace/ChangeNamespace.cpp
@@ -555,17 +555,16 @@ void ChangeNamespaceTool::moveOldNamespace(
   if (Decl::castToDeclContext(NsDecl)->decls_empty())
     return;
 
+  const SourceManager &SM = *Result.SourceManager;
   // Get the range of the code in the old namespace.
-  SourceLocation Start = getLocAfterNamespaceLBrace(
-      NsDecl, *Result.SourceManager, Result.Context->getLangOpts());
+  SourceLocation Start =
+      getLocAfterNamespaceLBrace(NsDecl, SM, Result.Context->getLangOpts());
   assert(Start.isValid() && "Can't find l_brace for namespace.");
-  SourceLocation End = NsDecl->getRBraceLoc().getLocWithOffset(-1);
-  // Create a replacement that deletes the code in the old namespace merely for
-  // retrieving offset and length from it.
-  const auto R = createReplacement(Start, End, "", *Result.SourceManager);
   MoveNamespace MoveNs;
-  MoveNs.Offset = R.getOffset();
-  MoveNs.Length = R.getLength();
+  MoveNs.Offset = SM.getFileOffset(Start);
+  // The range of the moved namespace is from the location just past the left
+  // brace to the location right before the right brace.
+  MoveNs.Length = SM.getFileOffset(NsDecl->getRBraceLoc()) - MoveNs.Offset;
 
   // Insert the new namespace after `DiffOldNamespace`. For example, if
   // `OldNamespace` is "a::b::c" and `NewNamespace` is `a::x::y`, then
@@ -577,18 +576,16 @@ void ChangeNamespaceTool::moveOldNamespace(
   const NamespaceDecl *OuterNs = getOuterNamespace(NsDecl, DiffOldNamespace);
   SourceLocation InsertionLoc = Start;
   if (OuterNs) {
-    SourceLocation LocAfterNs =
-        getStartOfNextLine(OuterNs->getRBraceLoc(), *Result.SourceManager,
-                           Result.Context->getLangOpts());
+    SourceLocation LocAfterNs = getStartOfNextLine(
+        OuterNs->getRBraceLoc(), SM, Result.Context->getLangOpts());
     assert(LocAfterNs.isValid() &&
            "Failed to get location after DiffOldNamespace");
     InsertionLoc = LocAfterNs;
   }
-  MoveNs.InsertionOffset = Result.SourceManager->getFileOffset(
-      Result.SourceManager->getSpellingLoc(InsertionLoc));
-  MoveNs.FID = Result.SourceManager->getFileID(Start);
+  MoveNs.InsertionOffset = SM.getFileOffset(SM.getSpellingLoc(InsertionLoc));
+  MoveNs.FID = SM.getFileID(Start);
   MoveNs.SourceMgr = Result.SourceManager;
-  MoveNamespaces[R.getFilePath()].push_back(MoveNs);
+  MoveNamespaces[SM.getFilename(Start)].push_back(MoveNs);
 }
 
 // Removes a class forward declaration from the code in the moved namespace and
diff --git a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
index 726e9dce455..a7034ad80ee 100644
--- a/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
+++ b/clang-tools-extra/unittests/change-namespace/ChangeNamespaceTests.cpp
@@ -110,12 +110,52 @@ TEST_F(ChangeNamespaceTest, NewNsNestedInOldNs) {
                          "namespace nc {\n"
                          "class A {};\n"
                          "} // namespace nc\n"
+                         "} // namespace nb\n"
+                         "} // namespace na\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
+TEST_F(ChangeNamespaceTest, NewNsNestedInOldNsWithSurroundingNewLines) {
+  NewNamespace = "na::nb::nc";
+  std::string Code = "namespace na {\n"
+                     "namespace nb {\n"
+                     "\n"
+                     "class A {};\n"
+                     "\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+  std::string Expected = "namespace na {\n"
+                         "namespace nb {\n"
+                         "namespace nc {\n"
+                         "\n"
+                         "class A {};\n"
                          "\n"
+                         "} // namespace nc\n"
                          "} // namespace nb\n"
                          "} // namespace na\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
 }
 
+TEST_F(ChangeNamespaceTest, MoveOldNsWithSurroundingNewLines) {
+  NewNamespace = "nx::ny";
+  std::string Code = "namespace na {\n"
+                     "namespace nb {\n"
+                     "\n"
+                     "class A {};\n"
+                     "\n"
+                     "} // namespace nb\n"
+                     "} // namespace na\n";
+  std::string Expected = "\n\n"
+                         "namespace nx {\n"
+                         "namespace ny {\n"
+                         "\n"
+                         "class A {};\n"
+                         "\n"
+                         "} // namespace ny\n"
+                         "} // namespace nx\n";
+  EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
+}
+
 TEST_F(ChangeNamespaceTest, NewNsNestedInOldNsWithRefs) {
   NewNamespace = "na::nb::nc";
   std::string Code = "namespace na {\n"
@@ -134,7 +174,6 @@ TEST_F(ChangeNamespaceTest, NewNsNestedInOldNsWithRefs) {
                          "class C {};\n"
                          "void f() { A a; B b; }\n"
                          "} // namespace nc\n"
-                         "\n"
                          "} // namespace nb\n"
                          "} // namespace na\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -1497,7 +1536,7 @@ TEST_F(ChangeNamespaceTest, UsingAliasInTemplate) {
                          "void f() {\n"
                          "  GG<float> g;\n"
                          "}\n"
-                         "} // namespace nc\n\n"
+                         "} // namespace nc\n"
                          "} // namespace nb\n"
                          "} // namespace na\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));
@@ -1554,7 +1593,7 @@ TEST_F(ChangeNamespaceTest, TemplateUsingAliasInBaseClass) {
                          "  struct Derived::Nested nested;\n"
                          "  const struct Derived::Nested *nested_ptr;\n"
                          "}\n"
-                         "} // namespace nc\n\n"
+                         "} // namespace nc\n"
                          "} // namespace nb\n"
                          "} // namespace na\n";
   EXPECT_EQ(format(Expected), runChangeNamespaceOnCode(Code));