<div dir="ltr">Hi Eric, <div><br></div><div>the asan bootstrap bot shows a leak in this newly added tests. Please fix or revert ASAP. </div><div><a href="http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/136/steps/check-clang%20asan/logs/stdio">http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/136/steps/check-clang%20asan/logs/stdio</a><br></div><div><pre style="font-family:"courier new",courier,monotype,monospace;color:rgb(0,0,0);font-size:medium"><span class="gmail-stdout">Direct leak of 720 byte(s) in 1 object(s) allocated from:
#0 0x6ace50 in operator new(unsigned long) /mnt/b/sanitizer-buildbot2/sanitizer-x86_64-linux-bootstrap/build/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:82
#1 0xc83f64 in clang::tooling::DeduplicateByFileTest_NonExistingFilePath_Test::TestBody() /mnt/b/sanitizer-buildbot2/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/unittests/Tooling/Re</span></pre></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Nov 6, 2016 at 10:08 PM, Eric Liu via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: ioeric<br>
Date: Mon Nov 7 00:08:23 2016<br>
New Revision: 286096<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=286096&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=286096&view=rev</a><br>
Log:<br>
Deduplicate replacements by FileEntry instead of file names.<br>
<br>
Summary:<br>
The current version does not deduplicate equivalent file paths correctly.<br>
For example, a relative path and an absolute path are considered inequivalent.<br>
Comparing FileEnry addresses these issues.<br>
<br>
Reviewers: djasper<br>
<br>
Subscribers: alexshap, klimek, cfe-commits<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D26288" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26288</a><br>
<br>
Modified:<br>
cfe/trunk/include/clang/<wbr>Tooling/Core/Replacement.h<br>
cfe/trunk/lib/Tooling/Core/<wbr>Replacement.cpp<br>
cfe/trunk/lib/Tooling/<wbr>Refactoring.cpp<br>
cfe/trunk/unittests/Tooling/<wbr>RefactoringTest.cpp<br>
<br>
Modified: cfe/trunk/include/clang/<wbr>Tooling/Core/Replacement.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=286096&r1=286095&r2=286096&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/include/<wbr>clang/Tooling/Core/<wbr>Replacement.h?rev=286096&r1=<wbr>286095&r2=286096&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/include/clang/<wbr>Tooling/Core/Replacement.h (original)<br>
+++ cfe/trunk/include/clang/<wbr>Tooling/Core/Replacement.h Mon Nov 7 00:08:23 2016<br>
@@ -19,6 +19,7 @@<br>
#ifndef LLVM_CLANG_TOOLING_CORE_<wbr>REPLACEMENT_H<br>
#define LLVM_CLANG_TOOLING_CORE_<wbr>REPLACEMENT_H<br>
<br>
+#include "clang/Basic/FileManager.h"<br>
#include "clang/Basic/LangOptions.h"<br>
#include "clang/Basic/SourceLocation.h"<br>
#include "llvm/ADT/StringRef.h"<br>
@@ -293,9 +294,10 @@ calculateRangesAfterReplacemen<wbr>ts(const R<br>
const std::vector<Range> &Ranges);<br>
<br>
/// \brief If there are multiple <File, Replacements> pairs with the same file<br>
-/// path after removing dots, we only keep one pair (with path after dots being<br>
-/// removed) and discard the rest.<br>
+/// entry, we only keep one pair and discard the rest.<br>
+/// If a file does not exist, its corresponding replacements will be ignored.<br>
std::map<std::string, Replacements> groupReplacementsByFile(<br>
+ FileManager &FileMgr,<br>
const std::map<std::string, Replacements> &FileToReplaces);<br>
<br>
template <typename Node><br>
<br>
Modified: cfe/trunk/lib/Tooling/Core/<wbr>Replacement.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=286096&r1=286095&r2=286096&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Tooling/<wbr>Core/Replacement.cpp?rev=<wbr>286096&r1=286095&r2=286096&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Tooling/Core/<wbr>Replacement.cpp (original)<br>
+++ cfe/trunk/lib/Tooling/Core/<wbr>Replacement.cpp Mon Nov 7 00:08:23 2016<br>
@@ -20,6 +20,7 @@<br>
#include "clang/Basic/SourceManager.h"<br>
#include "clang/Lex/Lexer.h"<br>
#include "clang/Rewrite/Core/Rewriter.<wbr>h"<br>
+#include "llvm/ADT/SmallPtrSet.h"<br>
#include "llvm/Support/FileSystem.h"<br>
#include "llvm/Support/Path.h"<br>
#include "llvm/Support/raw_os_ostream.<wbr>h"<br>
@@ -566,12 +567,16 @@ llvm::Expected<std::string> applyAllRepl<br>
}<br>
<br>
std::map<std::string, Replacements> groupReplacementsByFile(<br>
+ FileManager &FileMgr,<br>
const std::map<std::string, Replacements> &FileToReplaces) {<br>
std::map<std::string, Replacements> Result;<br>
+ llvm::SmallPtrSet<const FileEntry *, 16> ProcessedFileEntries;<br>
for (const auto &Entry : FileToReplaces) {<br>
- llvm::SmallString<256> CleanPath(Entry.first);<br>
- llvm::sys::path::remove_dots(<wbr>CleanPath, /*remove_dot_dot=*/true);<br>
- Result[CleanPath.str()] = std::move(Entry.second);<br>
+ const FileEntry *FE = FileMgr.getFile(Entry.first);<br>
+ if (!FE)<br>
+ llvm::errs() << "File path " << Entry.first << " is invalid.\n";<br>
+ else if (ProcessedFileEntries.insert(<wbr>FE).second)<br>
+ Result[Entry.first] = std::move(Entry.second);<br>
}<br>
return Result;<br>
}<br>
<br>
Modified: cfe/trunk/lib/Tooling/<wbr>Refactoring.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring.cpp?rev=286096&r1=286095&r2=286096&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/lib/Tooling/<wbr>Refactoring.cpp?rev=286096&r1=<wbr>286095&r2=286096&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/lib/Tooling/<wbr>Refactoring.cpp (original)<br>
+++ cfe/trunk/lib/Tooling/<wbr>Refactoring.cpp Mon Nov 7 00:08:23 2016<br>
@@ -57,7 +57,8 @@ int RefactoringTool::runAndSave(<wbr>Frontend<br>
<br>
bool RefactoringTool::<wbr>applyAllReplacements(Rewriter &Rewrite) {<br>
bool Result = true;<br>
- for (const auto &Entry : groupReplacementsByFile(<wbr>FileToReplaces))<br>
+ for (const auto &Entry : groupReplacementsByFile(<br>
+ Rewrite.getSourceMgr().<wbr>getFileManager(), FileToReplaces))<br>
Result = tooling::applyAllReplacements(<wbr>Entry.second, Rewrite) && Result;<br>
return Result;<br>
}<br>
@@ -73,7 +74,8 @@ bool formatAndApplyAllReplacements(<br>
FileManager &Files = SM.getFileManager();<br>
<br>
bool Result = true;<br>
- for (const auto &FileAndReplaces : groupReplacementsByFile(<wbr>FileToReplaces)) {<br>
+ for (const auto &FileAndReplaces : groupReplacementsByFile(<br>
+ Rewrite.getSourceMgr().<wbr>getFileManager(), FileToReplaces)) {<br>
const std::string &FilePath = FileAndReplaces.first;<br>
auto &CurReplaces = FileAndReplaces.second;<br>
<br>
<br>
Modified: cfe/trunk/unittests/Tooling/<wbr>RefactoringTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=286096&r1=286095&r2=286096&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/cfe/trunk/unittests/<wbr>Tooling/RefactoringTest.cpp?<wbr>rev=286096&r1=286095&r2=<wbr>286096&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- cfe/trunk/unittests/Tooling/<wbr>RefactoringTest.cpp (original)<br>
+++ cfe/trunk/unittests/Tooling/<wbr>RefactoringTest.cpp Mon Nov 7 00:08:23 2016<br>
@@ -19,6 +19,7 @@<br>
#include "clang/Basic/FileManager.h"<br>
#include "clang/Basic/LangOptions.h"<br>
#include "clang/Basic/SourceManager.h"<br>
+#include "clang/Basic/<wbr>VirtualFileSystem.h"<br>
#include "clang/Format/Format.h"<br>
#include "clang/Frontend/<wbr>CompilerInstance.h"<br>
#include "clang/Frontend/<wbr>FrontendAction.h"<br>
@@ -972,40 +973,64 @@ TEST_F(MergeReplacementsTest, Overlappin<br>
toReplacements({{"", 0, 3, "cc"}, {"", 3, 3, "dd"}}));<br>
}<br>
<br>
-TEST(DeduplicateByFileTest, LeaveLeadingDotDot) {<br>
+TEST(DeduplicateByFileTest, PathsWithDots) {<br>
std::map<std::string, Replacements> FileToReplaces;<br>
+ llvm::IntrusiveRefCntPtr<vfs::<wbr>InMemoryFileSystem> VFS(<br>
+ new vfs::InMemoryFileSystem());<br>
+ FileManager *FileMgr = new FileManager(FileSystemOptions(<wbr>), VFS);<br>
#if !defined(LLVM_ON_WIN32)<br>
- FileToReplaces["../../a/b/../.<wbr>/c.h"] = Replacements();<br>
- FileToReplaces["../../a/c.h"] = Replacements();<br>
+ StringRef Path1 = "a/b/.././c.h";<br>
+ StringRef Path2 = "a/c.h";<br>
#else<br>
- FileToReplaces["..\\..\\a\\b\\<wbr>..\\.\\c.h"] = Replacements();<br>
- FileToReplaces["..\\..\\a\\c.<wbr>h"] = Replacements();<br>
+ StringRef Path1 = "a\\b\\..\\.\\c.h";<br>
+ StringRef Path2 = "a\\c.h";<br>
#endif<br>
- FileToReplaces = groupReplacementsByFile(<wbr>FileToReplaces);<br>
+ EXPECT_TRUE(VFS->addFile(<wbr>Path1, 0, llvm::MemoryBuffer::<wbr>getMemBuffer("")));<br>
+ EXPECT_TRUE(VFS->addFile(<wbr>Path2, 0, llvm::MemoryBuffer::<wbr>getMemBuffer("")));<br>
+ FileToReplaces[Path1] = Replacements();<br>
+ FileToReplaces[Path2] = Replacements();<br>
+ FileToReplaces = groupReplacementsByFile(*<wbr>FileMgr, FileToReplaces);<br>
EXPECT_EQ(1u, FileToReplaces.size());<br>
-#if !defined(LLVM_ON_WIN32)<br>
- EXPECT_EQ("../../a/c.h", FileToReplaces.begin()->first)<wbr>;<br>
-#else<br>
- EXPECT_EQ("..\\..\\a\\c.h", FileToReplaces.begin()->first)<wbr>;<br>
-#endif<br>
+ EXPECT_EQ(Path1, FileToReplaces.begin()->first)<wbr>;<br>
}<br>
<br>
-TEST(DeduplicateByFileTest, RemoveDotSlash) {<br>
+TEST(DeduplicateByFileTest, PathWithDotSlash) {<br>
std::map<std::string, Replacements> FileToReplaces;<br>
+ llvm::IntrusiveRefCntPtr<vfs::<wbr>InMemoryFileSystem> VFS(<br>
+ new vfs::InMemoryFileSystem());<br>
+ FileManager *FileMgr = new FileManager(FileSystemOptions(<wbr>), VFS);<br>
#if !defined(LLVM_ON_WIN32)<br>
- FileToReplaces["./a/b/.././c.<wbr>h"] = Replacements();<br>
- FileToReplaces["a/c.h"] = Replacements();<br>
+ StringRef Path1 = "./a/b/c.h";<br>
+ StringRef Path2 = "a/b/c.h";<br>
#else<br>
- FileToReplaces[".\\a\\b\\..\\.<wbr>\\c.h"] = Replacements();<br>
- FileToReplaces["a\\c.h"] = Replacements();<br>
+ StringRef Path1 = ".\\a\\b\\c.h";<br>
+ StringRef Path2 = "a\\b\\c.h";<br>
#endif<br>
- FileToReplaces = groupReplacementsByFile(<wbr>FileToReplaces);<br>
+ EXPECT_TRUE(VFS->addFile(<wbr>Path1, 0, llvm::MemoryBuffer::<wbr>getMemBuffer("")));<br>
+ EXPECT_TRUE(VFS->addFile(<wbr>Path2, 0, llvm::MemoryBuffer::<wbr>getMemBuffer("")));<br>
+ FileToReplaces[Path1] = Replacements();<br>
+ FileToReplaces[Path2] = Replacements();<br>
+ FileToReplaces = groupReplacementsByFile(*<wbr>FileMgr, FileToReplaces);<br>
EXPECT_EQ(1u, FileToReplaces.size());<br>
+ EXPECT_EQ(Path1, FileToReplaces.begin()->first)<wbr>;<br>
+}<br>
+<br>
+TEST(DeduplicateByFileTest, NonExistingFilePath) {<br>
+ std::map<std::string, Replacements> FileToReplaces;<br>
+ llvm::IntrusiveRefCntPtr<vfs::<wbr>InMemoryFileSystem> VFS(<br>
+ new vfs::InMemoryFileSystem());<br>
+ FileManager *FileMgr = new FileManager(FileSystemOptions(<wbr>), VFS);<br>
#if !defined(LLVM_ON_WIN32)<br>
- EXPECT_EQ("a/c.h", FileToReplaces.begin()->first)<wbr>;<br>
+ StringRef Path1 = "./a/b/c.h";<br>
+ StringRef Path2 = "a/b/c.h";<br>
#else<br>
- EXPECT_EQ("a\\c.h", FileToReplaces.begin()->first)<wbr>;<br>
+ StringRef Path1 = ".\\a\\b\\c.h";<br>
+ StringRef Path2 = "a\\b\\c.h";<br>
#endif<br>
+ FileToReplaces[Path1] = Replacements();<br>
+ FileToReplaces[Path2] = Replacements();<br>
+ FileToReplaces = groupReplacementsByFile(*<wbr>FileMgr, FileToReplaces);<br>
+ EXPECT_TRUE(FileToReplaces.<wbr>empty());<br>
}<br>
<br>
} // end namespace tooling<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>