[clang-move] Make the output code look more pretty.

[hokein]

Summary

Add decent blank lines between declarations:

• Add extra blank line after #define or #includes.
• Add extra blank line between declarations.
• Add extra blank line in front of #endif.

Previously, the new generated code is quite tight:

#ifndef FOO_H
#define FOO_H
namespace a {
class A { public: int f(); };
int A::f() { return 0; }
} // namespace a
#endif // FOO_H

After this patch, the code looks like:

#ifndef FOO_H
#define FOO_H

namespace a {
class A { public: int f(); };

int A::f() { return 0; }
} // namespace a

#endif // FOO_H

[ioeric]

clang-move/ClangMove.cpp

341

It's been a while since the last time I reviewed this, and I need to struggle to understand what CurrentNamespaces, NextNamespaces, and IsNamespaceStart are... some comments for them are really appreciated.

[hokein]

Add comments and update patch descriptions.

[ioeric]

Lg with one nit.

clang-move/ClangMove.cpp

338–339

Wouldn't this create something like:

namespace x {
namespace y {
...
} // namespace y
  <----------------------------extra \n.
} // namespace x

[hokein]

clang-move/ClangMove.cpp

338–339

No. This code is for nested namespace. In your case, there should be a declaration after } // namespace y like:

namespace x {
namespace y {
...
} // namespace y

int A::f() {}
} // namespace x

[hokein]

clang-move/ClangMove.cpp

338–339

Oh, I see your meaning here. This case could be happened if the loop runs multiple times. Looking to it.

[hokein]

Fix a corner case, and add a test.

[hokein]

Remove a trailing blank line.

clang-move/ClangMove.cpp

createInsertedReplacements(const std::vector<std::string> &Includes,
   std::string GuardName(FileName);
   if (IsHeader) {
     for (size_t i = 0; i < GuardName.size(); ++i) {
       if (!isAlphanumeric(GuardName[i]))
         GuardName[i] = '_';
     }
     GuardName = StringRef(GuardName).upper();
     NewCode += "#ifndef " + GuardName + "\n";
-    NewCode += "#define " + GuardName + "\n";
+    NewCode += "#define " + GuardName + "\n\n";
   }
 
   // Add #Includes.
   for (const auto &Include : Includes)
     NewCode += Include;
 
   if (!Includes.empty())
     NewCode += "\n";
 
   // Add moved class definition and its related declarations. All declarations
   // in same namespace are grouped together.
+  //
+  // Record namespaces where the current position is in.
   std::vector<std::string> CurrentNamespaces;
   for (const auto &MovedDecl : Decls) {
+    // The namespaces of the declaration being moved.
     std::vector<std::string> DeclNamespaces = GetNamespaces(MovedDecl.Decl);
     auto CurrentIt = CurrentNamespaces.begin();
     auto DeclIt = DeclNamespaces.begin();
+    // Skip the common prefix.
     while (CurrentIt != CurrentNamespaces.end() &&
            DeclIt != DeclNamespaces.end()) {
       if (*CurrentIt != *DeclIt)
         break;
       ++CurrentIt;
       ++DeclIt;
     }
+    // Calculate the new namespaces after adding MovedDecl in CurrentNamespace,
+    // which is used for next iteration of this loop.
     std::vector<std::string> NextNamespaces(CurrentNamespaces.begin(),
                                             CurrentIt);
     NextNamespaces.insert(NextNamespaces.end(), DeclIt, DeclNamespaces.end());
+
+
+    // End with CurrentNamespace.
+    bool HasEndCurrentNamespace = false;
     auto RemainingSize = CurrentNamespaces.end() - CurrentIt;
     for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0;
          --RemainingSize, ++It) {
       assert(It < CurrentNamespaces.rend());
       NewCode += "} // namespace " + *It + "\n";
+      HasEndCurrentNamespace = true;
     }
+    // Add trailing '\n' after the nested namespace definition.
+    if (HasEndCurrentNamespace)
+      NewCode += "\n";
+
+    // If the moved declaration is not in CurrentNamespace, add extra namespace
+    // definitions.
+    bool IsInNewNamespace = false;
     while (DeclIt != DeclNamespaces.end()) {
       NewCode += "namespace " + *DeclIt + " {\n";
+      IsInNewNamespace = true;
       ++DeclIt;
     }
+    // If the moved declaration is in same namespace CurrentNamespace, add
+    // a preceeding `\n' before the moved declaration.
+    if (!IsInNewNamespace)
+      NewCode += "\n";
     NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM);
     CurrentNamespaces = std::move(NextNamespaces);
   }
   std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end());
   for (const auto &NS : CurrentNamespaces)
     NewCode += "} // namespace " + NS + "\n";
 
   if (IsHeader)
-    NewCode += "#endif // " + GuardName + "\n";
+    NewCode += "\n#endif // " + GuardName + "\n";
   return clang::tooling::Replacements(
       clang::tooling::Replacement(FileName, 0, 0, NewCode));
 }
 
 } // namespace
 
 std::unique_ptr<clang::ASTConsumer>
 ClangMoveAction::CreateASTConsumer(clang::CompilerInstance &Compiler,

unittests/clang-move/ClangMoveTests.cpp

const char ExpectedTestCC[] = "#include \"foo.h\"\n"
                               "  f1();\n"
                               "  return 1;\n"
                               "}\n"
                               "} // namespace b\n"
                               "} // namespace a\n";
 
 const char ExpectedNewHeader[] = "#ifndef NEW_FOO_H\n"
                                  "#define NEW_FOO_H\n"
+                                 "\n"
                                  "namespace a {\n"
                                  "class C1; // test\n"
+                                 "\n"
                                  "template <typename T> class C2;\n"
                                  "namespace b {\n"
                                  "// This is a Foo class\n"
                                  "// which is used in\n"
                                  "// test.\n"
                                  "class Foo {\n"
                                  "public:\n"
                                  "  void f();\n"
                                  "\n"
                                  "private:\n"
                                  "  C1 *c1;\n"
                                  "  static int b;\n"
                                  "}; // abc\n"
                                  "} // namespace b\n"
                                  "} // namespace a\n"
+                                 "\n"
                                  "#endif // NEW_FOO_H\n";
 
 const char ExpectedNewCC[] = "namespace a {\n"
                              "namespace b {\n"
                              "namespace {\n"
                              "// comment1.\n"
                              "void f1() {}\n"
                              "/// comment2.\n"
                              "int kConstInt1 = 0;\n"
                              "} // namespace\n"
+                             "\n"
                              "/* comment 3*/\n"
                              "static int kConstInt2 = 1;\n"
+                             "\n"
                              "/** comment4\n"
                              "*/\n"
                              "static int help() {\n"
                              "  int a = 0;\n"
                              "  return a;\n"
                              "}\n"
+                             "\n"
                              "// comment5\n"
                              "// comment5\n"
                              "void Foo::f() { f1(); }\n"
+                             "\n"
                              "/////////////\n"
                              "// comment //\n"
                              "/////////////\n"
                              "int Foo::b = 2;\n"
                              "} // namespace b\n"
                              "} // namespace a\n";
 
 std::map<std::string, std::string>
for (const auto& Header : TestHeaders) {
     EXPECT_EQ("", Results[Spec.OldHeader]);
     EXPECT_EQ("", Results[Spec.OldCC]);
   }
 }
 
 TEST(ClangMove, DontMoveAll) {
   const char ExpectedHeader[] = "#ifndef NEW_FOO_H\n"
                                 "#define NEW_FOO_H\n"
+                                "\n"
                                 "class A {\npublic:\n  int f();\n};\n"
+                                "\n"
                                 "#endif // NEW_FOO_H\n";
   const char Code[] = "#include \"foo.h\"\nint A::f() { return 0; }";
   std::vector<std::string> TestHeaders = {
-    "typedef int Int;\nclass A {\npublic:\n  int f();\n};",
-    "using Int=int;\nclass A {\npublic:\n  int f();\n};",
-    "class B {};\nclass A {\npublic:\n  int f();\n};",
-    "void f() {};\nclass A {\npublic:\n  int f();\n};",
-    "enum Color { RED };\nclass A {\npublic:\n  int f();\n};",
+    "typedef int Int;\nclass A {\npublic:\n  int f();\n};\n",
+    "using Int=int;\nclass A {\npublic:\n  int f();\n};\n",
+    "class B {};\nclass A {\npublic:\n  int f();\n};\n",
+    "void f() {};\nclass A {\npublic:\n  int f();\n};\n",
+    "enum Color { RED };\nclass A {\npublic:\n  int f();\n};\n",
   };
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Names.push_back("A");
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   for (const auto& Header : TestHeaders) {
     auto Results = runClangMoveOnCode(Spec, Header.c_str(), Code);
     EXPECT_EQ(ExpectedHeader, Results[Spec.NewHeader]);
     // The expected old header should not contain class A definition.
-    std::string ExpectedOldHeader = Header.substr(0, Header.size() - 31);
+    std::string ExpectedOldHeader = Header.substr(0, Header.size() - 32);
     EXPECT_EQ(ExpectedOldHeader, Results[Spec.OldHeader]);
   }
 }
 
 TEST(ClangMove, MacroInFunction) {
   const char TestHeader[] = "#define INT int\n"
                             "class A {\npublic:\n  int f();\n};\n"
                             "class B {};\n";
   const char TestCode[] = "#include \"foo.h\"\n"
                           "INT A::f() { return 0; }\n";
   const char ExpectedNewCode[] = "#include \"new_foo.h\"\n\n"
                                  "INT A::f() { return 0; }\n";
   move::ClangMoveTool::MoveDefinitionSpec Spec;
   Spec.Names.push_back("A");
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   auto Results = runClangMoveOnCode(Spec, TestHeader, TestCode);
   EXPECT_EQ(ExpectedNewCode, Results[Spec.NewCC]);
 }
 
+TEST(ClangMove, WellFormattedCode) {
+  const std::string CommonHeader =
+      "namespace a {\n"
+      "namespace b {\n"
+      "namespace c {\n"
+      "class C;\n"
+      "\n"
+      "class A {\npublic:\n  void f();\n  void f2();\n};\n"
+      "} // namespace c\n"
+      "} // namespace b\n"
+      "\n"
+      "namespace d {\n"
+      "namespace e {\n"
+      "class B {\npublic:\n  void f();\n};\n"
+      "} // namespace e\n"
+      "} // namespace d\n"
+      "} // namespace a\n";
+  const std::string CommonCode = "\n"
+                                 "namespace a {\n"
+                                 "namespace b {\n"
+                                 "namespace c {\n"
+                                 "void A::f() {}\n"
+                                 "\n"
+                                 "void A::f2() {}\n"
+                                 "} // namespace c\n"
+                                 "} // namespace b\n"
+                                 "\n"
+                                 "namespace d {\n"
+                                 "namespace e {\n"
+                                 "void B::f() {}\n"
+                                 "} // namespace e\n"
+                                 "} // namespace d\n"
+                                 "} // namespace a\n";
+  // Add dummy class to prevent behavior of moving all declarations from header.
+  const std::string TestHeader = CommonHeader + "class D {};\n";
+  const std::string TestCode = "#include \"foo.h\"\n" + CommonCode;
+  const std::string ExpectedNewHeader = "#ifndef NEW_FOO_H\n"
+                                        "#define NEW_FOO_H\n"
+                                        "\n" +
+                                        CommonHeader +
+                                        "\n"
+                                        "#endif // NEW_FOO_H\n";
+  const std::string ExpectedNewCC = "#include \"new_foo.h\"\n" + CommonCode;
+  move::ClangMoveTool::MoveDefinitionSpec Spec;
+  Spec.Names.push_back("a::b::c::A");
+  Spec.Names.push_back("a::d::e::B");
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
+  Spec.NewCC = "new_foo.cc";
+  auto Results = runClangMoveOnCode(Spec, TestHeader.c_str(), TestCode.c_str());
+  EXPECT_EQ(ExpectedNewCC, Results[Spec.NewCC]);
+  EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
+}
+
 } // namespace
 } // namespce move
 } // namespace clang