hokein created this revision.
hokein added a reviewer: ioeric.
hokein added subscribers: bkramer, cfe-commits.
Herald added subscribers: mgorny, beanz.
This patch is kind of reverting r283202 (but keep the updated lint test),
because it leads to a few annoying problems by comparing absolute file path to
the relative path:
- MakeAbsolutePath does wrong things with symlinks.
- Removing `..` directory is not always safe.
https://reviews.llvm.org/D25508
Files:
clang-move/ClangMove.cpp
clang-move/ClangMove.h
clang-move/tool/ClangMoveMain.cpp
test/CMakeLists.txt
test/clang-move/Inputs/database_template.json
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
@@ -187,12 +187,8 @@
CreateFiles(Spec.OldCC, TestCC);
std::map<std::string, tooling::Replacements> FileToReplacements;
- llvm::SmallString<128> InitialDirectory;
- std::error_code EC = llvm::sys::fs::current_path(InitialDirectory);
- assert(!EC);
- (void)EC;
auto Factory = llvm::make_unique<clang::move::ClangMoveActionFactory>(
- Spec, FileToReplacements, InitialDirectory.str(), "LLVM");
+ Spec, FileToReplacements, "LLVM");
tooling::runToolOnCodeWithArgs(
Factory->create(), TestCC, {"-std=c++11", "-fparse-all-comments"},
Index: test/clang-move/move-class.cpp
===================================================================
--- test/clang-move/move-class.cpp
+++ test/clang-move/move-class.cpp
@@ -1,21 +1,15 @@
// RUN: mkdir -p %T/clang-move/build
+// RUN: mkdir -p %T/clang-move/include
// RUN: sed 's|$test_dir|%/T/clang-move|g' %S/Inputs/database_template.json > %T/clang-move/compile_commands.json
-// RUN: cp %S/Inputs/test* %T/clang-move/
-// RUN: touch %T/clang-move/test2.h
-// RUN: cd %T/clang-move
-// RUN: clang-move -name="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=../clang-move/test.cpp -old_header=../clang-move/test.h %T/clang-move/test.cpp
+// RUN: cp %S/Inputs/test.h %T/clang-move/include
+// RUN: cp %S/Inputs/test.cpp %T/clang-move/
+// RUN: touch %T/clang-move/include/test2.h
+// RUN: cd %T/clang-move/build
+// RUN: clang-move -name="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=test.cpp -old_header=../include/test.h %T/clang-move/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/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
-// RUN: FileCheck -input-file=%T/clang-move/test.h %s -implicit-check-not='{{namespace.*}}'
-//
-// RUN: cp %S/Inputs/test* %T/clang-move/
-// RUN: cd %T/clang-move
-// RUN: clang-move -name="a::Foo" -new_cc=%T/clang-move/new_test.cpp -new_header=%T/clang-move/new_test.h -old_cc=%T/clang-move/test.cpp -old_header=%T/clang-move/test.h %T/clang-move/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/test.cpp -check-prefix=CHECK-OLD-TEST-CPP %s
-// RUN: FileCheck -input-file=%T/clang-move/test.h %s -implicit-check-not='{{namespace.*}}'
+// RUN: FileCheck -input-file=%T/clang-move/include/test.h %s -implicit-check-not='{{namespace.*}}'
//
// CHECK-NEW-TEST-H: namespace a {
// CHECK-NEW-TEST-H: class Foo {
Index: test/clang-move/Inputs/database_template.json
===================================================================
--- test/clang-move/Inputs/database_template.json
+++ test/clang-move/Inputs/database_template.json
@@ -1,7 +1,7 @@
[
{
"directory": "$test_dir/build",
- "command": "clang++ -o test.o $test_dir/test.cpp",
+ "command": "clang++ -o test.o -I../include $test_dir/test.cpp",
"file": "$test_dir/test.cpp"
}
]
Index: test/CMakeLists.txt
===================================================================
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -44,7 +44,6 @@
clang-apply-replacements
clang-change-namespace
clang-include-fixer
- clang-move
clang-query
clang-rename
clang-reorder-fields
Index: clang-move/tool/ClangMoveMain.cpp
===================================================================
--- clang-move/tool/ClangMoveMain.cpp
+++ clang-move/tool/ClangMoveMain.cpp
@@ -17,15 +17,13 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Process.h"
#include "llvm/Support/YAMLTraits.h"
-#include "llvm/Support/Path.h"
#include <set>
#include <string>
using namespace clang;
using namespace llvm;
namespace {
-
std::error_code CreateNewFile(const llvm::Twine &path) {
int fd = 0;
if (std::error_code ec =
@@ -40,22 +38,19 @@
cl::opt<std::string> Name("name", cl::desc("The name of class being moved."),
cl::cat(ClangMoveCategory));
-cl::opt<std::string>
- OldHeader("old_header",
- cl::desc("The relative/absolute file path of old header."),
- cl::cat(ClangMoveCategory));
+cl::opt<std::string> OldHeader("old_header", cl::desc("Old header name."),
+ cl::cat(ClangMoveCategory));
-cl::opt<std::string>
- OldCC("old_cc", cl::desc("The relative/absolute file path of old cc."),
- cl::cat(ClangMoveCategory));
+cl::opt<std::string> OldCC("old_cc", cl::desc("Old CC file name."),
+ cl::cat(ClangMoveCategory));
cl::opt<std::string>
NewHeader("new_header",
cl::desc("The relative/absolute file path of new header."),
cl::cat(ClangMoveCategory));
cl::opt<std::string>
- NewCC("new_cc", cl::desc("The relative/absolute file path of new cc."),
+ NewCC("new_cc", cl::desc("The relative/absolute file path of new cc file."),
cl::cat(ClangMoveCategory));
cl::opt<std::string>
@@ -91,15 +86,8 @@
Spec.NewHeader = NewHeader;
Spec.OldCC = OldCC;
Spec.NewCC = NewCC;
-
- llvm::SmallString<128> InitialDirectory;
- if (std::error_code EC = llvm::sys::fs::current_path(InitialDirectory))
- llvm::report_fatal_error("Cannot detect current path: " +
- Twine(EC.message()));
-
auto Factory = llvm::make_unique<clang::move::ClangMoveActionFactory>(
- Spec, Tool.getReplacements(), InitialDirectory.str(), Style);
-
+ Spec, Tool.getReplacements(), Style);
int CodeStatus = Tool.run(Factory.get());
if (CodeStatus)
return CodeStatus;
Index: clang-move/ClangMove.h
===================================================================
--- clang-move/ClangMove.h
+++ clang-move/ClangMove.h
@@ -39,9 +39,9 @@
struct MoveDefinitionSpec {
// A fully qualified name, e.g. "X", "a::X".
std::string Name;
- // The file path of old header, can be relative path and absolute path.
+ // The old header name.
std::string OldHeader;
- // The file path of old cc, can be relative path and absolute path.
+ // The old cc name.
std::string OldCC;
// The file path of new header, can be relative path and absolute path.
std::string NewHeader;
@@ -52,7 +52,7 @@
ClangMoveTool(
const MoveDefinitionSpec &MoveSpec,
std::map<std::string, tooling::Replacements> &FileToReplacements,
- llvm::StringRef OriginalRunningDirectory, llvm::StringRef Style);
+ llvm::StringRef Style);
void registerMatchers(ast_matchers::MatchFinder *Finder);
@@ -71,9 +71,7 @@
/// \param SM The SourceManager.
void addIncludes(llvm::StringRef IncludeHeader,
bool IsAngled,
- llvm::StringRef SearchPath,
- llvm::StringRef FileName,
- const SourceManager& SM);
+ llvm::StringRef FileName);
private:
void removeClassDefinitionInOldFiles();
@@ -91,12 +89,6 @@
std::vector<std::string> HeaderIncludes;
// The #includes in old_cc.cc.
std::vector<std::string> CCIncludes;
- // The original working directory where the local clang-move binary runs.
- //
- // clang-move will change its current working directory to the build
- // directory when analyzing the source file. We save the original working
- // directory in order to get the absolute file path for the fields in Spec.
- std::string OriginalRunningDirectory;
// The name of a predefined code style.
std::string FallbackStyle;
};
@@ -106,9 +98,8 @@
ClangMoveAction(
const ClangMoveTool::MoveDefinitionSpec &spec,
std::map<std::string, tooling::Replacements> &FileToReplacements,
- llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)
- : MoveTool(spec, FileToReplacements, OriginalRunningDirectory,
- FallbackStyle) {
+ llvm::StringRef FallbackStyle)
+ : MoveTool(spec, FileToReplacements, FallbackStyle) {
MoveTool.registerMatchers(&MatchFinder);
}
@@ -128,20 +119,17 @@
ClangMoveActionFactory(
const ClangMoveTool::MoveDefinitionSpec &Spec,
std::map<std::string, tooling::Replacements> &FileToReplacements,
- llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)
+ llvm::StringRef FallbackStyle)
: Spec(Spec), FileToReplacements(FileToReplacements),
- OriginalRunningDirectory(OriginalRunningDirectory),
FallbackStyle(FallbackStyle) {}
clang::FrontendAction *create() override {
- return new ClangMoveAction(Spec, FileToReplacements,
- OriginalRunningDirectory, FallbackStyle);
+ return new ClangMoveAction(Spec, FileToReplacements, FallbackStyle);
}
private:
const ClangMoveTool::MoveDefinitionSpec &Spec;
std::map<std::string, tooling::Replacements> &FileToReplacements;
- std::string OriginalRunningDirectory;
std::string FallbackStyle;
};
Index: clang-move/ClangMove.cpp
===================================================================
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -16,62 +16,13 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Rewrite/Core/Rewriter.h"
#include "clang/Tooling/Core/Replacement.h"
-#include "llvm/Support/Path.h"
using namespace clang::ast_matchers;
namespace clang {
namespace move {
namespace {
-// Make the Path absolute using the CurrentDir if the Path is not an absolute
-// path. An empty Path will result in an empty string.
-std::string MakeAbsolutePath(StringRef CurrentDir, StringRef Path) {
- if (Path.empty())
- return "";
- llvm::SmallString<128> InitialDirectory(CurrentDir);
- llvm::SmallString<128> AbsolutePath(Path);
- if (std::error_code EC =
- llvm::sys::fs::make_absolute(InitialDirectory, AbsolutePath))
- llvm::errs() << "Warning: could not make absolute file: '" << EC.message()
- << '\n';
- llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
- llvm::sys::path::native(AbsolutePath);
- return AbsolutePath.str();
-}
-
-// Make the Path absolute using the current working directory of the given
-// SourceManager if the Path is not an absolute path.
-//
-// The Path can be a path relative to the build directory, or retrieved from
-// the SourceManager.
-std::string MakeAbsolutePath(const SourceManager& SM, StringRef Path) {
- llvm::SmallString<128> AbsolutePath(Path);
- if (std::error_code EC =
- SM.getFileManager().getVirtualFileSystem()->makeAbsolute(AbsolutePath))
- llvm::errs() << "Warning: could not make absolute file: '" << EC.message()
- << '\n';
- llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
- llvm::sys::path::native(AbsolutePath);
- return AbsolutePath.str();
-}
-
-// Matches AST nodes that are expanded within the given AbsoluteFilePath.
-AST_POLYMORPHIC_MATCHER_P(isExpansionInFile,
- AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc),
- std::string, AbsoluteFilePath) {
- auto &SourceManager = Finder->getASTContext().getSourceManager();
- auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
- if (ExpansionLoc.isInvalid())
- return false;
- auto FileEntry =
- SourceManager.getFileEntryForID(SourceManager.getFileID(ExpansionLoc));
- if (!FileEntry)
- return false;
- return MakeAbsolutePath(SourceManager, FileEntry->getName()) ==
- AbsoluteFilePath;
-}
-
class FindAllIncludes : public clang::PPCallbacks {
public:
explicit FindAllIncludes(SourceManager *SM, ClangMoveTool *const MoveTool)
@@ -82,11 +33,10 @@
StringRef FileName, bool IsAngled,
clang::CharSourceRange /*FilenameRange*/,
const clang::FileEntry * /*File*/,
- StringRef SearchPath, StringRef /*RelativePath*/,
+ 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);
+ MoveTool->addIncludes(FileName, IsAngled, FileEntry->getName());
}
private:
@@ -173,19 +123,16 @@
}
}
-bool isInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D,
- llvm::StringRef OriginalRunningDirectory,
- llvm::StringRef OldHeader) {
- if (OldHeader.empty())
+bool IsInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D,
+ llvm::StringRef HeaderFile) {
+ if (HeaderFile.empty())
return false;
auto ExpansionLoc = SM.getExpansionLoc(D->getLocStart());
if (ExpansionLoc.isInvalid())
return false;
- if (const auto *FE = SM.getFileEntryForID(SM.getFileID(ExpansionLoc))) {
- return MakeAbsolutePath(SM, FE->getName()) ==
- MakeAbsolutePath(OriginalRunningDirectory, OldHeader);
- }
+ if (const auto *FE = SM.getFileEntryForID(SM.getFileID(ExpansionLoc)))
+ return llvm::StringRef(FE->getName()).endswith(HeaderFile);
return false;
}
@@ -280,24 +227,22 @@
return MatchFinder.newASTConsumer();
}
+
ClangMoveTool::ClangMoveTool(
const MoveDefinitionSpec &MoveSpec,
std::map<std::string, tooling::Replacements> &FileToReplacements,
- llvm::StringRef OriginalRunningDirectory, llvm::StringRef FallbackStyle)
+ llvm::StringRef FallbackStyle)
: Spec(MoveSpec), FileToReplacements(FileToReplacements),
- OriginalRunningDirectory(OriginalRunningDirectory),
FallbackStyle(FallbackStyle) {
Spec.Name = llvm::StringRef(Spec.Name).ltrim(':');
if (!Spec.NewHeader.empty())
CCIncludes.push_back("#include \"" + Spec.NewHeader + "\"\n");
}
void ClangMoveTool::registerMatchers(ast_matchers::MatchFinder *Finder) {
std::string FullyQualifiedName = "::" + Spec.Name;
- auto InOldHeader = isExpansionInFile(
- MakeAbsolutePath(OriginalRunningDirectory, Spec.OldHeader));
- auto InOldCC = isExpansionInFile(
- MakeAbsolutePath(OriginalRunningDirectory, Spec.OldCC));
+ auto InOldHeader = isExpansionInFileMatching(Spec.OldHeader);
+ auto InOldCC = isExpansionInFileMatching(Spec.OldCC);
auto InOldFiles = anyOf(InOldHeader, InOldCC);
auto InMovedClass =
hasDeclContext(cxxRecordDecl(hasName(FullyQualifiedName)));
@@ -377,31 +322,22 @@
}
}
-void ClangMoveTool::addIncludes(llvm::StringRef IncludeHeader,
- bool IsAngled,
- llvm::StringRef SearchPath,
- llvm::StringRef FileName,
- const SourceManager& SM) {
- auto AbsoluteSearchPath = MakeAbsolutePath(SM, SearchPath);
+void ClangMoveTool::addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
+ llvm::StringRef FileName) {
// FIXME: Add old.h to the new.cc/h when the new target has dependencies on
// old.h/c. For instance, when moved class uses another class defined in
// old.h, the old.h should be added in new.h.
- if (MakeAbsolutePath(OriginalRunningDirectory, Spec.OldHeader) ==
- MakeAbsolutePath(AbsoluteSearchPath, IncludeHeader))
+ if (!Spec.OldHeader.empty() &&
+ llvm::StringRef(Spec.OldHeader).endswith(IncludeHeader))
return;
std::string IncludeLine =
IsAngled ? ("#include <" + IncludeHeader + ">\n").str()
: ("#include \"" + IncludeHeader + "\"\n").str();
-
- std::string AbsolutePath = MakeAbsolutePath(SM, FileName);
- if (MakeAbsolutePath(OriginalRunningDirectory, Spec.OldHeader) ==
- AbsolutePath) {
+ if (!Spec.OldHeader.empty() && FileName.endswith(Spec.OldHeader))
HeaderIncludes.push_back(IncludeLine);
- } else if (MakeAbsolutePath(OriginalRunningDirectory, Spec.OldCC) ==
- AbsolutePath) {
+ else if (!Spec.OldCC.empty() && FileName.endswith(Spec.OldCC))
CCIncludes.push_back(IncludeLine);
- }
}
void ClangMoveTool::removeClassDefinitionInOldFiles() {
@@ -434,8 +370,7 @@
std::vector<MovedDecl> NewHeaderDecls;
std::vector<MovedDecl> NewCCDecls;
for (const auto &MovedDecl : MovedDecls) {
- if (isInHeaderFile(*MovedDecl.SM, MovedDecl.Decl, OriginalRunningDirectory,
- Spec.OldHeader))
+ if (IsInHeaderFile(*MovedDecl.SM, MovedDecl.Decl, Spec.OldHeader))
NewHeaderDecls.push_back(MovedDecl);
else
NewCCDecls.push_back(MovedDecl);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
