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

[ioeric]

Summary

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

[alexander-shaposhnikov]

change-namespace/ChangeNamespace.cpp

563

maybe i am missing smth, to be honest i somehow think this code is "hacky" / looks like a "workaround", so don't like the idea of creating a replacement only for extracting the offset / filepath.
I would prefer to refactor this (+ SourceManager ?) to get rid of these hacks (they create extra "tech debt" plus decrease the readability)

[ioeric]

• Get rid of hacky replacement.

change-namespace/ChangeNamespace.cpp

563

You are right; I was being lazy... remove the hack ;-)

[alexander-shaposhnikov]

thx, LGTM

[hokein]

LGTM with some nits.

Would be clearer to elaborate more descriptions in the commit message. Looks like the patch actually resolves newline character "\n" rather than whitespace...

change-namespace/ChangeNamespace.cpp

570

Maybe use MoveNs.Offset instead of recalling SM.getFileOffset(Start);

[ioeric]

• fix a nit.

change-namespace/ChangeNamespace.cpp

void ChangeNamespaceTool::moveOldNamespace(
   // If the namespace is empty, do nothing.
   if (Decl::castToDeclContext(NsDecl)->decls_empty())
     return;
 
   // Get the range of the code in the old namespace.
   SourceLocation Start = getLocAfterNamespaceLBrace(
       NsDecl, *Result.SourceManager, 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);
+  // Create a replacement merely for retrieving file path and start offset.
+  const auto R = createReplacement(Start, Start, "", *Result.SourceManager);
   MoveNamespace MoveNs;
   MoveNs.Offset = R.getOffset();
-  MoveNs.Length = R.getLength();
+  // The replacement contains token range, but we need to be more accurate and
+  // calculate offsets directly. Note that the range is from the location just
+  // past the left brace to the location right before the right brace.
+  MoveNs.Length = Result.SourceManager->getFileOffset(NsDecl->getRBraceLoc()) -
+                  Result.SourceManager->getFileOffset(Start);
 
   // Insert the new namespace after `DiffOldNamespace`. For example, if
   // `OldNamespace` is "a::b::c" and `NewNamespace` is `a::x::y`, then
   // "x::y" will be inserted inside the existing namespace "a" and after "a::b".
   // `OuterNs` is the first namespace in `DiffOldNamespace`, e.g. "namespace b"
   // in the above example.
   // If there is no outer namespace (i.e. DiffOldNamespace is empty), the new
   // namespace will be a nested namespace in the old namespace.

unittests/change-namespace/ChangeNamespaceTests.cpp

std::string Code = "namespace na {\n"
                      "class A {};\n"
                      "} // namespace nb\n"
                      "} // namespace na\n";
   std::string Expected = "namespace na {\n"
                          "namespace nb {\n"
                          "namespace nc {\n"
                          "class A {};\n"
                          "} // namespace nc\n"
-                         "\n"
-                         "} // namespace nb\n"
-                         "} // namespace na\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"
+  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"
                      "class A {};\n"
                      "namespace nb {\n"
                      "class B {};\n"
                      "class C {};\n"
                      "void f() { A a; B b; }\n"
                      "} // namespace nb\n"
                      "} // namespace na\n";
   std::string Expected = "namespace na {\n"
                          "class A {};\n"
                          "namespace nb {\n"
                          "namespace nc {\n"
                          "class B {};\n"
                          "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));
 }
 
 TEST_F(ChangeNamespaceTest, SimpleMoveIntoAnotherNestedNamespace) {
   NewNamespace = "na::nc";
   std::string Code = "namespace na {\n"
std::string Expected = "namespace some_ns {\n"
                          "using GG = some_ns::G<int, P>;\n"
                          "} // namespace na\n"
                          "namespace na {\n"
                          "namespace nb {\n"
                          "namespace nc {\n"
                          "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));
 }
 
 TEST_F(ChangeNamespaceTest, TemplateUsingAliasInBaseClass) {
   NewNamespace = "na::nb::nc";
   std::string Code = "namespace some_ns {\n"
std::string Expected = "namespace some_ns {\n"
                          "namespace nc {\n"
                          "void f() {\n"
                          "  Derived::GG<float> g;\n"
                          "  const Derived::GG<int> gg;\n"
                          "  const Derived::GG<int>* gg_ptr;\n"
                          "  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));
 }
 
 } // anonymous namespace
 } // namespace change_namespace
 } // namespace clang