hokein updated this revision to Diff 77820.
hokein marked an inline comment as done.
hokein added a comment.
Fix a corner case, and add a test.
https://reviews.llvm.org/D26493
Files:
clang-move/ClangMove.cpp
unittests/clang-move/ClangMoveTests.cpp
Index: unittests/clang-move/ClangMoveTests.cpp
===================================================================
--- unittests/clang-move/ClangMoveTests.cpp
+++ unittests/clang-move/ClangMoveTests.cpp
@@ -127,8 +127,10 @@
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"
@@ -144,6 +146,7 @@
"}; // abc\n"
"} // namespace b\n"
"} // namespace a\n"
+ "\n"
"#endif // NEW_FOO_H\n";
const char ExpectedNewCC[] = "namespace a {\n"
@@ -154,17 +157,21 @@
"/// 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"
@@ -312,15 +319,17 @@
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");
@@ -332,7 +341,7 @@
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]);
}
}
@@ -355,6 +364,62 @@
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
Index: clang-move/ClangMove.cpp
===================================================================
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -294,7 +294,7 @@
}
GuardName = StringRef(GuardName).upper();
NewCode += "#ifndef " + GuardName + "\n";
- NewCode += "#define " + GuardName + "\n";
+ NewCode += "#define " + GuardName + "\n\n";
}
// Add #Includes.
@@ -306,40 +306,63 @@
// 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));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
