From cf2913c6f12fbade57640c3410eb95d05baa27e5 Mon Sep 17 00:00:00 2001 From: Eric Liu Date: Mon, 7 Nov 2016 06:08:23 +0000 Subject: [PATCH] Deduplicate replacements by FileEntry instead of file names. Summary: The current version does not deduplicate equivalent file paths correctly. For example, a relative path and an absolute path are considered inequivalent. Comparing FileEnry addresses these issues. Reviewers: djasper Subscribers: alexshap, klimek, cfe-commits Differential Revision: https://reviews.llvm.org/D26288 llvm-svn: 286096 --- clang/include/clang/Tooling/Core/Replacement.h | 6 ++- clang/lib/Tooling/Core/Replacement.cpp | 11 +++-- clang/lib/Tooling/Refactoring.cpp | 6 ++- clang/unittests/Tooling/RefactoringTest.cpp | 63 ++++++++++++++++++-------- 4 files changed, 60 insertions(+), 26 deletions(-) diff --git a/clang/include/clang/Tooling/Core/Replacement.h b/clang/include/clang/Tooling/Core/Replacement.h index 2ac180b..1442beb 100644 --- a/clang/include/clang/Tooling/Core/Replacement.h +++ b/clang/include/clang/Tooling/Core/Replacement.h @@ -19,6 +19,7 @@ #ifndef LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H #define LLVM_CLANG_TOOLING_CORE_REPLACEMENT_H +#include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" #include "llvm/ADT/StringRef.h" @@ -293,9 +294,10 @@ calculateRangesAfterReplacements(const Replacements &Replaces, const std::vector &Ranges); /// \brief If there are multiple pairs with the same file -/// path after removing dots, we only keep one pair (with path after dots being -/// removed) and discard the rest. +/// entry, we only keep one pair and discard the rest. +/// If a file does not exist, its corresponding replacements will be ignored. std::map groupReplacementsByFile( + FileManager &FileMgr, const std::map &FileToReplaces); template diff --git a/clang/lib/Tooling/Core/Replacement.cpp b/clang/lib/Tooling/Core/Replacement.cpp index ab80c90..ff12ab7 100644 --- a/clang/lib/Tooling/Core/Replacement.cpp +++ b/clang/lib/Tooling/Core/Replacement.cpp @@ -20,6 +20,7 @@ #include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" #include "clang/Rewrite/Core/Rewriter.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" #include "llvm/Support/raw_os_ostream.h" @@ -566,12 +567,16 @@ llvm::Expected applyAllReplacements(StringRef Code, } std::map groupReplacementsByFile( + FileManager &FileMgr, const std::map &FileToReplaces) { std::map Result; + llvm::SmallPtrSet ProcessedFileEntries; for (const auto &Entry : FileToReplaces) { - llvm::SmallString<256> CleanPath(Entry.first); - llvm::sys::path::remove_dots(CleanPath, /*remove_dot_dot=*/true); - Result[CleanPath.str()] = std::move(Entry.second); + const FileEntry *FE = FileMgr.getFile(Entry.first); + if (!FE) + llvm::errs() << "File path " << Entry.first << " is invalid.\n"; + else if (ProcessedFileEntries.insert(FE).second) + Result[Entry.first] = std::move(Entry.second); } return Result; } diff --git a/clang/lib/Tooling/Refactoring.cpp b/clang/lib/Tooling/Refactoring.cpp index 241557d..308c1ac 100644 --- a/clang/lib/Tooling/Refactoring.cpp +++ b/clang/lib/Tooling/Refactoring.cpp @@ -57,7 +57,8 @@ int RefactoringTool::runAndSave(FrontendActionFactory *ActionFactory) { bool RefactoringTool::applyAllReplacements(Rewriter &Rewrite) { bool Result = true; - for (const auto &Entry : groupReplacementsByFile(FileToReplaces)) + for (const auto &Entry : groupReplacementsByFile( + Rewrite.getSourceMgr().getFileManager(), FileToReplaces)) Result = tooling::applyAllReplacements(Entry.second, Rewrite) && Result; return Result; } @@ -73,7 +74,8 @@ bool formatAndApplyAllReplacements( FileManager &Files = SM.getFileManager(); bool Result = true; - for (const auto &FileAndReplaces : groupReplacementsByFile(FileToReplaces)) { + for (const auto &FileAndReplaces : groupReplacementsByFile( + Rewrite.getSourceMgr().getFileManager(), FileToReplaces)) { const std::string &FilePath = FileAndReplaces.first; auto &CurReplaces = FileAndReplaces.second; diff --git a/clang/unittests/Tooling/RefactoringTest.cpp b/clang/unittests/Tooling/RefactoringTest.cpp index 81f0f1d..31b14ea 100644 --- a/clang/unittests/Tooling/RefactoringTest.cpp +++ b/clang/unittests/Tooling/RefactoringTest.cpp @@ -19,6 +19,7 @@ #include "clang/Basic/FileManager.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceManager.h" +#include "clang/Basic/VirtualFileSystem.h" #include "clang/Format/Format.h" #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/FrontendAction.h" @@ -972,40 +973,64 @@ TEST_F(MergeReplacementsTest, OverlappingRanges) { toReplacements({{"", 0, 3, "cc"}, {"", 3, 3, "dd"}})); } -TEST(DeduplicateByFileTest, LeaveLeadingDotDot) { +TEST(DeduplicateByFileTest, PathsWithDots) { std::map FileToReplaces; + llvm::IntrusiveRefCntPtr VFS( + new vfs::InMemoryFileSystem()); + FileManager *FileMgr = new FileManager(FileSystemOptions(), VFS); #if !defined(LLVM_ON_WIN32) - FileToReplaces["../../a/b/.././c.h"] = Replacements(); - FileToReplaces["../../a/c.h"] = Replacements(); + StringRef Path1 = "a/b/.././c.h"; + StringRef Path2 = "a/c.h"; #else - FileToReplaces["..\\..\\a\\b\\..\\.\\c.h"] = Replacements(); - FileToReplaces["..\\..\\a\\c.h"] = Replacements(); + StringRef Path1 = "a\\b\\..\\.\\c.h"; + StringRef Path2 = "a\\c.h"; #endif - FileToReplaces = groupReplacementsByFile(FileToReplaces); + EXPECT_TRUE(VFS->addFile(Path1, 0, llvm::MemoryBuffer::getMemBuffer(""))); + EXPECT_TRUE(VFS->addFile(Path2, 0, llvm::MemoryBuffer::getMemBuffer(""))); + FileToReplaces[Path1] = Replacements(); + FileToReplaces[Path2] = Replacements(); + FileToReplaces = groupReplacementsByFile(*FileMgr, FileToReplaces); EXPECT_EQ(1u, FileToReplaces.size()); -#if !defined(LLVM_ON_WIN32) - EXPECT_EQ("../../a/c.h", FileToReplaces.begin()->first); -#else - EXPECT_EQ("..\\..\\a\\c.h", FileToReplaces.begin()->first); -#endif + EXPECT_EQ(Path1, FileToReplaces.begin()->first); } -TEST(DeduplicateByFileTest, RemoveDotSlash) { +TEST(DeduplicateByFileTest, PathWithDotSlash) { std::map FileToReplaces; + llvm::IntrusiveRefCntPtr VFS( + new vfs::InMemoryFileSystem()); + FileManager *FileMgr = new FileManager(FileSystemOptions(), VFS); #if !defined(LLVM_ON_WIN32) - FileToReplaces["./a/b/.././c.h"] = Replacements(); - FileToReplaces["a/c.h"] = Replacements(); + StringRef Path1 = "./a/b/c.h"; + StringRef Path2 = "a/b/c.h"; #else - FileToReplaces[".\\a\\b\\..\\.\\c.h"] = Replacements(); - FileToReplaces["a\\c.h"] = Replacements(); + StringRef Path1 = ".\\a\\b\\c.h"; + StringRef Path2 = "a\\b\\c.h"; #endif - FileToReplaces = groupReplacementsByFile(FileToReplaces); + EXPECT_TRUE(VFS->addFile(Path1, 0, llvm::MemoryBuffer::getMemBuffer(""))); + EXPECT_TRUE(VFS->addFile(Path2, 0, llvm::MemoryBuffer::getMemBuffer(""))); + FileToReplaces[Path1] = Replacements(); + FileToReplaces[Path2] = Replacements(); + FileToReplaces = groupReplacementsByFile(*FileMgr, FileToReplaces); EXPECT_EQ(1u, FileToReplaces.size()); + EXPECT_EQ(Path1, FileToReplaces.begin()->first); +} + +TEST(DeduplicateByFileTest, NonExistingFilePath) { + std::map FileToReplaces; + llvm::IntrusiveRefCntPtr VFS( + new vfs::InMemoryFileSystem()); + FileManager *FileMgr = new FileManager(FileSystemOptions(), VFS); #if !defined(LLVM_ON_WIN32) - EXPECT_EQ("a/c.h", FileToReplaces.begin()->first); + StringRef Path1 = "./a/b/c.h"; + StringRef Path2 = "a/b/c.h"; #else - EXPECT_EQ("a\\c.h", FileToReplaces.begin()->first); + StringRef Path1 = ".\\a\\b\\c.h"; + StringRef Path2 = "a\\b\\c.h"; #endif + FileToReplaces[Path1] = Replacements(); + FileToReplaces[Path2] = Replacements(); + FileToReplaces = groupReplacementsByFile(*FileMgr, FileToReplaces); + EXPECT_TRUE(FileToReplaces.empty()); } } // end namespace tooling -- 2.7.4