r286096 - Deduplicate replacements by FileEntry instead of file names.
Eric Liu via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 7 10:40:59 PST 2016
Thanks Kostya! I'll look into this now.
On Mon, Nov 7, 2016 at 10:39 AM Kostya Serebryany <kcc at google.com> wrote:
> Hi Eric,
>
> the asan bootstrap bot shows a leak in this newly added tests. Please fix
> or revert ASAP.
>
> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap/builds/136/steps/check-clang%20asan/logs/stdio
>
> 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
>
>
> On Sun, Nov 6, 2016 at 10:08 PM, Eric Liu via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
> Author: ioeric
> Date: Mon Nov 7 00:08:23 2016
> New Revision: 286096
>
> URL: http://llvm.org/viewvc/llvm-project?rev=286096&view=rev
> Log:
> 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
>
> Modified:
> cfe/trunk/include/clang/Tooling/Core/Replacement.h
> cfe/trunk/lib/Tooling/Core/Replacement.cpp
> cfe/trunk/lib/Tooling/Refactoring.cpp
> cfe/trunk/unittests/Tooling/RefactoringTest.cpp
>
> Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=286096&r1=286095&r2=286096&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original)
> +++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Mon Nov 7 00:08:23
> 2016
> @@ -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 R
> 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.
> +/// If a file does not exist, its corresponding replacements will be
> ignored.
> std::map<std::string, Replacements> groupReplacementsByFile(
> + FileManager &FileMgr,
> const std::map<std::string, Replacements> &FileToReplaces);
>
> template <typename Node>
>
> Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=286096&r1=286095&r2=286096&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original)
> +++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Mon Nov 7 00:08:23 2016
> @@ -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<std::string> applyAllRepl
> }
>
> 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);
> - 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;
> }
>
> Modified: cfe/trunk/lib/Tooling/Refactoring.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Refactoring.cpp?rev=286096&r1=286095&r2=286096&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Tooling/Refactoring.cpp (original)
> +++ cfe/trunk/lib/Tooling/Refactoring.cpp Mon Nov 7 00:08:23 2016
> @@ -57,7 +57,8 @@ int RefactoringTool::runAndSave(Frontend
>
> 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;
>
>
> Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=286096&r1=286095&r2=286096&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original)
> +++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Mon Nov 7 00:08:23
> 2016
> @@ -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, Overlappin
> toReplacements({{"", 0, 3, "cc"}, {"", 3, 3, "dd"}}));
> }
>
> -TEST(DeduplicateByFileTest, LeaveLeadingDotDot) {
> +TEST(DeduplicateByFileTest, PathsWithDots) {
> std::map<std::string, Replacements> FileToReplaces;
> + llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> 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<std::string, Replacements> FileToReplaces;
> + llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> 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<std::string, Replacements> FileToReplaces;
> + llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> 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
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161107/ea794497/attachment-0001.html>
More information about the cfe-commits
mailing list