hokein updated this revision to Diff 77208.
hokein marked 2 inline comments as done.
hokein added a comment.
Fix remaining comments.
https://reviews.llvm.org/D26236
Files:
clang-move/ClangMove.cpp
clang-move/ClangMove.h
test/clang-move/Inputs/test.h
test/clang-move/move-class.cpp
unittests/clang-move/ClangMoveTests.cpp
Index: unittests/clang-move/ClangMoveTests.cpp
===================================================================
--- unittests/clang-move/ClangMoveTests.cpp
+++ unittests/clang-move/ClangMoveTests.cpp
@@ -173,24 +173,25 @@
"} // namespace a\n";
std::map<std::string, std::string>
-runClangMoveOnCode(const move::ClangMoveTool::MoveDefinitionSpec &Spec) {
+runClangMoveOnCode(const move::ClangMoveTool::MoveDefinitionSpec &Spec,
+ const char *const Header = TestHeader,
+ const char *const CC = TestCC) {
clang::RewriterTestContext Context;
std::map<llvm::StringRef, clang::FileID> FileToFileID;
std::vector<std::pair<std::string, std::string>> FileToSourceText = {
- {TestHeaderName, TestHeader}, {TestCCName, TestCC}};
+ {TestHeaderName, Header}, {TestCCName, CC}};
auto CreateFiles = [&FileToSourceText, &Context, &FileToFileID](
llvm::StringRef Name, llvm::StringRef Code) {
if (!Name.empty()) {
- FileToSourceText.emplace_back(Name, Code);
FileToFileID[Name] = Context.createInMemoryFile(Name, Code);
}
};
CreateFiles(Spec.NewCC, "");
CreateFiles(Spec.NewHeader, "");
- CreateFiles(Spec.OldHeader, TestHeader);
- CreateFiles(Spec.OldCC, TestCC);
+ CreateFiles(Spec.OldHeader, Header);
+ CreateFiles(Spec.OldCC, CC);
std::map<std::string, tooling::Replacements> FileToReplacements;
llvm::SmallString<128> InitialDirectory;
@@ -201,7 +202,7 @@
Spec, FileToReplacements, InitialDirectory.str(), "LLVM");
tooling::runToolOnCodeWithArgs(
- Factory->create(), TestCC, {"-std=c++11", "-fparse-all-comments"},
+ Factory->create(), CC, {"-std=c++11", "-fparse-all-comments"},
TestCCName, "clang-move", std::make_shared<PCHContainerOperations>(),
FileToSourceText);
formatAndApplyAllReplacements(FileToReplacements, Context.Rewrite, "llvm");
@@ -263,6 +264,72 @@
EXPECT_EQ(0u, Results.size());
}
+TEST(ClangMove, MoveAll) {
+ std::vector<std::string> TestHeaders = {
+ "class A {\npublic:\n int f();\n};",
+ // forward declaration.
+ "class B;\nclass A {\npublic:\n int f();\n};",
+ // template forward declaration.
+ "template <typename T> class B;\nclass A {\npublic:\n int f();\n};",
+ "namespace a {}\nclass A {\npublic:\n int f();\n};",
+ "namespace a {}\nusing namespace a;\nclass A {\npublic:\n int f();\n};",
+ };
+ const char Code[] = "#include \"foo.h\"\nint A::f() { return 0; }";
+ 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(Header, Results[Spec.NewHeader]);
+ }
+}
+
+TEST(ClangMove, MoveAllMultipleClasses) {
+ move::ClangMoveTool::MoveDefinitionSpec Spec;
+ std::vector<std::string> TestHeaders = {
+ "class C;\nclass A {\npublic:\n int f();\n};\nclass B {};",
+ "class C;\nclass B;\nclass A {\npublic:\n int f();\n};\nclass B {};",
+ };
+ const char Code[] = "#include \"foo.h\"\nint A::f() { return 0; }";
+ Spec.Names = {std::string("A"), std::string("B")};
+ 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(Header, Results[Spec.NewHeader]);
+ }
+}
+
+TEST(ClangMove, DontMoveAll) {
+ const char ExpectedHeader[] = "#ifndef NEW_FOO_H\n"
+ "#define NEW_FOO_H\n"
+ "class A {\npublic:\n int f();\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};",
+ };
+ 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]);
+ }
+}
+
} // namespace
} // namespce move
} // namespace clang
Index: test/clang-move/move-class.cpp
===================================================================
--- test/clang-move/move-class.cpp
+++ test/clang-move/move-class.cpp
@@ -9,36 +9,35 @@
// RUN: clang-move -names="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=../src/test.cpp -old_header=../include/test.h %T/clang-move/src/test.cpp
// RUN: FileCheck -input-file=%T/clang-move/new_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
// RUN: FileCheck -input-file=%T/clang-move/new_test.h -check-prefix=CHECK-NEW-TEST-H %s
-// RUN: FileCheck -input-file=%T/clang-move/src/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
-// RUN: FileCheck -input-file=%T/clang-move/include/test.h %s -implicit-check-not='{{namespace.*}}'
+// RUN: FileCheck -input-file=%T/clang-move/src/test.cpp -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
+// RUN: FileCheck -input-file=%T/clang-move/include/test.h -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
//
// RUN: cp %S/Inputs/test.h %T/clang-move/include
// RUN: cp %S/Inputs/test.cpp %T/clang-move/src
// RUN: cd %T/clang-move/build
// RUN: clang-move -names="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=%T/clang-move/src/test.cpp -old_header=%T/clang-move/include/test.h %T/clang-move/src/test.cpp
// RUN: FileCheck -input-file=%T/clang-move/new_test.cpp -check-prefix=CHECK-NEW-TEST-CPP %s
// RUN: FileCheck -input-file=%T/clang-move/new_test.h -check-prefix=CHECK-NEW-TEST-H %s
-// RUN: FileCheck -input-file=%T/clang-move/src/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
-// RUN: FileCheck -input-file=%T/clang-move/include/test.h %s -implicit-check-not='{{namespace.*}}'
+// RUN: FileCheck -input-file=%T/clang-move/src/test.cpp -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
+// RUN: FileCheck -input-file=%T/clang-move/include/test.h -check-prefix=CHECK-OLD-TEST-EMPTY -allow-empty %s
//
//
-// CHECK-NEW-TEST-H: #ifndef {{.*}}CLANG_MOVE_NEW_TEST_H
-// CHECK-NEW-TEST-H: #define {{.*}}CLANG_MOVE_NEW_TEST_H
+// CHECK-NEW-TEST-H: #ifndef TEST_H // comment 1
+// CHECK-NEW-TEST-H: #define TEST_H
// CHECK-NEW-TEST-H: namespace a {
// CHECK-NEW-TEST-H: class Foo {
// CHECK-NEW-TEST-H: public:
// CHECK-NEW-TEST-H: int f();
// CHECK-NEW-TEST-H: int f2(int a, int b);
// CHECK-NEW-TEST-H: };
// CHECK-NEW-TEST-H: } // namespace a
-// CHECK-NEW-TEST-H: #endif // {{.*}}CLANG_MOVE_NEW_TEST_H
+// CHECK-NEW-TEST-H: #endif // TEST_H
//
// CHECK-NEW-TEST-CPP: #include "{{.*}}new_test.h"
// CHECK-NEW-TEST-CPP: #include "test2.h"
// CHECK-NEW-TEST-CPP: namespace a {
// CHECK-NEW-TEST-CPP: int Foo::f() { return 0; }
// CHECK-NEW-TEST-CPP: int Foo::f2(int a, int b) { return a + b; }
// CHECK-NEW-TEST-CPP: } // namespace a
//
-// CHECK-OLD-TEST-CPP: #include "test.h"
-// CHECK-OLD-TEST-CPP: #include "test2.h"
+// CHECK-OLD-TEST-EMPTY: {{^}}{{$}}
Index: test/clang-move/Inputs/test.h
===================================================================
--- test/clang-move/Inputs/test.h
+++ test/clang-move/Inputs/test.h
@@ -1,7 +1,10 @@
+#ifndef TEST_H // comment 1
+#define TEST_H
namespace a {
class Foo {
public:
int f();
int f2(int a, int b);
};
} // namespace a
+#endif // TEST_H
Index: clang-move/ClangMove.h
===================================================================
--- clang-move/ClangMove.h
+++ clang-move/ClangMove.h
@@ -14,6 +14,7 @@
#include "clang/Frontend/FrontendAction.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include <map>
#include <string>
#include <vector>
@@ -23,6 +24,9 @@
// FIXME: Make it support more types, e.g. function definitions.
// Currently only support moving class definition.
+//
+// When moving all class declarations in old header, all code in old.h/cc will
+// be moved.
class ClangMoveTool : public ast_matchers::MatchFinder::MatchCallback {
public:
// Information about the declaration being moved.
@@ -68,16 +72,20 @@
/// \param SearchPath The search path which was used to find the IncludeHeader
/// in the file system. It can be a relative path or an absolute path.
/// \param FileName The name of file where the IncludeHeader comes from.
+ /// \param IncludeRange The source range for the written file name in #include
+ /// (i.e. "old.h" for #include "old.h") in old.cc.
/// \param SM The SourceManager.
void addIncludes(llvm::StringRef IncludeHeader,
bool IsAngled,
llvm::StringRef SearchPath,
llvm::StringRef FileName,
+ clang::CharSourceRange IncludeFilenameRange,
const SourceManager& SM);
private:
void removeClassDefinitionInOldFiles();
void moveClassDefinitionToNewFiles();
+ void moveAll(SourceManager& SM, StringRef OldFile, StringRef NewFile);
MoveDefinitionSpec Spec;
// The Key is file path, value is the replacements being applied to the file.
@@ -99,6 +107,12 @@
std::string OriginalRunningDirectory;
// The name of a predefined code style.
std::string FallbackStyle;
+ // The unmoved named declarations in old header.
+ llvm::SmallPtrSet<const NamedDecl*, 8> UnremovedDeclsInOldHeader;
+ /// The source range for the written file name in #include (i.e. "old.h" for
+ /// #include "old.h") in old.cc, including the enclosing quotes or angle
+ /// brackets.
+ clang::CharSourceRange OldHeaderIncludeRange;
};
class ClangMoveAction : public clang::ASTFrontendAction {
Index: clang-move/ClangMove.cpp
===================================================================
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -121,13 +121,13 @@
void InclusionDirective(clang::SourceLocation HashLoc,
const clang::Token & /*IncludeTok*/,
StringRef FileName, bool IsAngled,
- clang::CharSourceRange /*FilenameRange*/,
+ clang::CharSourceRange FilenameRange,
const clang::FileEntry * /*File*/,
StringRef SearchPath, StringRef /*RelativePath*/,
const clang::Module * /*Imported*/) override {
if (const auto *FileEntry = SM.getFileEntryForID(SM.getFileID(HashLoc)))
MoveTool->addIncludes(FileName, IsAngled, SearchPath,
- FileEntry->getName(), SM);
+ FileEntry->getName(), FilenameRange, SM);
}
private:
@@ -331,20 +331,47 @@
auto InMovedClass =
hasOutermostEnclosingClass(cxxRecordDecl(*InMovedClassNames));
+ auto ForwardDecls =
+ cxxRecordDecl(unless(anyOf(isImplicit(), isDefinition())));
+
+ //============================================================================
+ // Matchers for old header
+ //============================================================================
+ // Match all top-level named declarations (e.g. function, variable, enum) in
+ // old header, exclude forward class declarations and namespace declarations.
+ //
+ // The old header which contains only one declaration being moved and forward
+ // declarations is considered to be moved totally.
+ auto AllDeclsInHeader = namedDecl(
+ unless(ForwardDecls), unless(namespaceDecl()),
+ unless(usingDirectiveDecl()), // using namespace decl.
+ unless(classTemplateDecl(has(ForwardDecls))), // template forward decl.
+ InOldHeader,
+ hasParent(decl(anyOf(namespaceDecl(), translationUnitDecl()))));
+ Finder->addMatcher(AllDeclsInHeader.bind("decls_in_header"), this);
+ // Match forward declarations in old header.
+ Finder->addMatcher(namedDecl(ForwardDecls, InOldHeader).bind("fwd_decl"),
+ this);
+
+ //============================================================================
+ // Matchers for old files, including old.h/old.cc
+ //============================================================================
// Match moved class declarations.
auto MovedClass = cxxRecordDecl(
InOldFiles, *InMovedClassNames, isDefinition(),
hasDeclContext(anyOf(namespaceDecl(), translationUnitDecl())));
Finder->addMatcher(MovedClass.bind("moved_class"), this);
-
// Match moved class methods (static methods included) which are defined
// outside moved class declaration.
Finder->addMatcher(
cxxMethodDecl(InOldFiles, ofOutermostEnclosingClass(*InMovedClassNames),
isDefinition())
.bind("class_method"),
this);
+ //============================================================================
+ // Matchers for old cc
+ //============================================================================
// Match static member variable definition of the moved class.
Finder->addMatcher(varDecl(InMovedClass, InOldCC, isDefinition(),
isStaticDataMember())
@@ -378,16 +405,13 @@
varDecl(IsOldCCStaticDefinition)))
.bind("static_decls"),
this);
-
- // Match forward declarations in old header.
- Finder->addMatcher(
- cxxRecordDecl(unless(anyOf(isImplicit(), isDefinition())), InOldHeader)
- .bind("fwd_decl"),
- this);
}
void ClangMoveTool::run(const ast_matchers::MatchFinder::MatchResult &Result) {
- if (const auto *CMD =
+ if (const auto *D =
+ Result.Nodes.getNodeAs<clang::NamedDecl>("decls_in_header")) {
+ UnremovedDeclsInOldHeader.insert(D);
+ } else if (const auto *CMD =
Result.Nodes.getNodeAs<clang::CXXMethodDecl>("class_method")) {
// Skip inline class methods. isInline() ast matcher doesn't ignore this
// case.
@@ -403,6 +427,7 @@
Result.Nodes.getNodeAs<clang::CXXRecordDecl>("moved_class")) {
MovedDecls.emplace_back(class_decl, &Result.Context->getSourceManager());
RemovedDecls.push_back(MovedDecls.back());
+ UnremovedDeclsInOldHeader.erase(class_decl);
} else if (const auto *FWD =
Result.Nodes.getNodeAs<clang::CXXRecordDecl>("fwd_decl")) {
// Skip all forwad declarations which appear after moved class declaration.
@@ -429,6 +454,7 @@
bool IsAngled,
llvm::StringRef SearchPath,
llvm::StringRef FileName,
+ clang::CharSourceRange IncludeFilenameRange,
const SourceManager& SM) {
SmallVector<char, 128> HeaderWithSearchPath;
llvm::sys::path::append(HeaderWithSearchPath, SearchPath, IncludeHeader);
@@ -439,8 +465,10 @@
// old.h, the old.h should be added in new.h.
if (AbsoluteOldHeader ==
MakeAbsolutePath(SM, llvm::StringRef(HeaderWithSearchPath.data(),
- HeaderWithSearchPath.size())))
+ HeaderWithSearchPath.size()))) {
+ OldHeaderIncludeRange = IncludeFilenameRange;
return;
+ }
std::string IncludeLine =
IsAngled ? ("#include <" + IncludeHeader + ">\n").str()
@@ -504,9 +532,47 @@
createInsertedReplacements(CCIncludes, NewCCDecls, Spec.NewCC);
}
+// Move all contents from OldFile to NewFile.
+void ClangMoveTool::moveAll(SourceManager &SM, StringRef OldFile,
+ StringRef NewFile) {
+ const FileEntry *FE = SM.getFileManager().getFile(
+ MakeAbsolutePath(OriginalRunningDirectory, OldFile));
+ if (!FE) {
+ llvm::errs() << "Failed to get file: " << OldFile << "\n";
+ return;
+ }
+ FileID ID = SM.getOrCreateFileID(FE, SrcMgr::C_User);
+ auto Begin = SM.getLocForStartOfFile(ID);
+ auto End = SM.getLocForEndOfFile(ID);
+ clang::tooling::Replacement RemoveAll (
+ SM, clang::CharSourceRange::getCharRange(Begin, End), "");
+ std::string FilePath = RemoveAll.getFilePath().str();
+ FileToReplacements[FilePath] = clang::tooling::Replacements(RemoveAll);
+
+ StringRef Code = SM.getBufferData(ID);
+ if (!NewFile.empty()) {
+ auto AllCode = clang::tooling::Replacements(
+ clang::tooling::Replacement(NewFile, 0, 0, Code));
+ // If we are moving from old.cc, an extra step is required: excluding
+ // the #include of "old.h", instead, we replace it with #include of "new.h".
+ if (Spec.NewCC == NewFile && OldHeaderIncludeRange.isValid()) {
+ AllCode = AllCode.merge(
+ clang::tooling::Replacements(clang::tooling::Replacement(
+ SM, OldHeaderIncludeRange, '"' + Spec.NewHeader + '"')));
+ }
+ FileToReplacements[NewFile] = std::move(AllCode);
+ }
+}
+
void ClangMoveTool::onEndOfTranslationUnit() {
if (RemovedDecls.empty())
return;
+ if (UnremovedDeclsInOldHeader.empty() && !Spec.OldHeader.empty()) {
+ auto &SM = *RemovedDecls[0].SM;
+ moveAll(SM, Spec.OldHeader, Spec.NewHeader);
+ moveAll(SM, Spec.OldCC, Spec.NewCC);
+ return;
+ }
removeClassDefinitionInOldFiles();
moveClassDefinitionToNewFiles();
}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
