[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 ↗

(On Diff #83023)

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 ↗

(On Diff #83023)

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

568 ↗

(On Diff #83024)

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

[ioeric]

• fix a nit.

clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp

 // the actual movement to `onEndOfTranslationUnit()`.
 void ChangeNamespaceTool::moveOldNamespace(
     const ast_matchers::MatchFinder::MatchResult &Result,
     const NamespaceDecl *NsDecl) {
   // If the namespace is empty, do nothing.
   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
   // "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.
   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
 // creates an `InsertForwardDeclaration` to insert the forward declaration back
 // into the old namespace after moving code from the old namespace to the new
 // namespace.
 // For example, changing "a" to "x":
 // Old code:

clang-tools-extra/trunk/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