ioeric created this revision.
ioeric added a reviewer: djasper.
ioeric added a subscriber: cfe-commits.
Herald added a subscriber: klimek.
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.
https://reviews.llvm.org/D26288
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
lib/Tooling/Refactoring.cpp
unittests/Tooling/RefactoringTest.cpp
Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -972,39 +972,41 @@
toReplacements({{"", 0, 3, "cc"}, {"", 3, 3, "dd"}}));
}
-TEST(DeduplicateByFileTest, LeaveLeadingDotDot) {
+TEST(DeduplicateByFileTest, PathsWithDots) {
std::map<std::string, Replacements> FileToReplaces;
+ FileManager *FileMgr = new FileManager(FileSystemOptions());
#if !defined(LLVM_ON_WIN32)
FileToReplaces["../../a/b/.././c.h"] = Replacements();
FileToReplaces["../../a/c.h"] = Replacements();
#else
FileToReplaces["..\\..\\a\\b\\..\\.\\c.h"] = Replacements();
FileToReplaces["..\\..\\a\\c.h"] = Replacements();
#endif
- FileToReplaces = groupReplacementsByFile(FileToReplaces);
+ FileToReplaces = groupReplacementsByFile(*FileMgr, FileToReplaces);
EXPECT_EQ(1u, FileToReplaces.size());
#if !defined(LLVM_ON_WIN32)
- EXPECT_EQ("../../a/c.h", FileToReplaces.begin()->first);
+ EXPECT_EQ("../../a/b/.././c.h", FileToReplaces.begin()->first);
#else
- EXPECT_EQ("..\\..\\a\\c.h", FileToReplaces.begin()->first);
+ EXPECT_EQ("..\\..\\a\\b\\..\\.\\c.h", FileToReplaces.begin()->first);
#endif
}
-TEST(DeduplicateByFileTest, RemoveDotSlash) {
+TEST(DeduplicateByFileTest, PathWithDotSlash) {
std::map<std::string, Replacements> FileToReplaces;
+ FileManager *FileMgr = new FileManager(FileSystemOptions());
#if !defined(LLVM_ON_WIN32)
- FileToReplaces["./a/b/.././c.h"] = Replacements();
- FileToReplaces["a/c.h"] = Replacements();
+ FileToReplaces["./a/b/c.h"] = Replacements();
+ FileToReplaces["a/b/c.h"] = Replacements();
#else
- FileToReplaces[".\\a\\b\\..\\.\\c.h"] = Replacements();
- FileToReplaces["a\\c.h"] = Replacements();
+ FileToReplaces[".\\a\\b\\c.h"] = Replacements();
+ FileToReplaces["a\\b\\c.h"] = Replacements();
#endif
- FileToReplaces = groupReplacementsByFile(FileToReplaces);
+ FileToReplaces = groupReplacementsByFile(*FileMgr, FileToReplaces);
EXPECT_EQ(1u, FileToReplaces.size());
#if !defined(LLVM_ON_WIN32)
- EXPECT_EQ("a/c.h", FileToReplaces.begin()->first);
+ EXPECT_EQ("./a/b/c.h", FileToReplaces.begin()->first);
#else
- EXPECT_EQ("a\\c.h", FileToReplaces.begin()->first);
+ EXPECT_EQ(".\\a\\b\\c.h", FileToReplaces.begin()->first);
#endif
}
Index: lib/Tooling/Refactoring.cpp
===================================================================
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -57,7 +57,8 @@
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 @@
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;
Index: lib/Tooling/Core/Replacement.cpp
===================================================================
--- lib/Tooling/Core/Replacement.cpp
+++ 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 @@
}
std::map<std::string, Replacements> groupReplacementsByFile(
+ FileManager &FileMgr,
const std::map<std::string, Replacements> &FileToReplaces) {
std::map<std::string, Replacements> Result;
+ llvm::SmallPtrSet<const FileEntry *, 16> ProcessedFileEntries;
for (const auto &Entry : FileToReplaces) {
- llvm::SmallString<256> CleanPath(Entry.first.data());
- 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 (ProcessedFileEntries.count(FE) != 0)
+ continue;
+ ProcessedFileEntries.insert(FE);
+ Result[Entry.first] = std::move(Entry.second);
}
return Result;
}
Index: include/clang/Tooling/Core/Replacement.h
===================================================================
--- include/clang/Tooling/Core/Replacement.h
+++ 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,9 @@
const std::vector<Range> &Ranges);
/// \brief If there are multiple <File, Replacements> 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.
std::map<std::string, Replacements> groupReplacementsByFile(
+ FileManager &FileMgr,
const std::map<std::string, Replacements> &FileToReplaces);
template <typename Node>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
