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