ilya-biryukov added inline comments.
================ Comment at: lib/Basic/VirtualFileSystem.cpp:478 +public: + InMemoryNode(const std::string& FileName, InMemoryNodeKind Kind) + : Kind(Kind), FileName(llvm::sys::path::filename(FileName.data())) {} ---------------- NIT: accept a parameter of type `llvm::StringRef` instead of `const std::string&`. ================ Comment at: lib/Basic/VirtualFileSystem.cpp:495 + InMemoryFile(Status Stat, std::unique_ptr<llvm::MemoryBuffer> Buffer) + : InMemoryNode(Stat.getName().str(), IME_File), Stat(std::move(Stat)), + Buffer(std::move(Buffer)) {} ---------------- NIT: `.str()` is redundant (in fact, a pessimization) if parameter of `InMemoryNode` is a `StringRef`. ================ Comment at: lib/Basic/VirtualFileSystem.cpp:522 + InMemoryHardLink(StringRef Path, const InMemoryFile &ResolvedFile) + : InMemoryNode(Path.str(), IME_HardLink), ResolvedFile(ResolvedFile) {} + const InMemoryFile &getResolvedFile() const { return ResolvedFile; } ---------------- NIT: as noted in the other comment, `.str()` can be removed if parameter type is changed. ================ Comment at: lib/Basic/VirtualFileSystem.cpp:570 InMemoryDirectory(Status Stat) - : InMemoryNode(std::move(Stat), IME_Directory) {} + : InMemoryNode(Stat.getName().str(), IME_Directory), + Stat(std::move(Stat)) {} ---------------- NIT: `.str()` can be removed ================ Comment at: lib/Basic/VirtualFileSystem.cpp:662 const auto ResolvedPerms = Perms.getValueOr(sys::fs::all_all); + // Cannot create HardLink from a directory. + if (HardLinkTarget && Buffer) ---------------- The comment looks outdated ================ Comment at: lib/Basic/VirtualFileSystem.cpp:663 + // Cannot create HardLink from a directory. + if (HardLinkTarget && Buffer) + return false; ---------------- Shouldn't we assert this instead? This is not an invalid input, this is actually breaking our program logic, right? I.e. one shouldn't call addFile with HardLinkTarget set and non-null buffer ================ Comment at: lib/Basic/VirtualFileSystem.cpp:677 + if (HardLinkTarget) + Child.reset(new detail::InMemoryHardLink(P.str(), *HardLinkTarget)); + else { ---------------- NIT: `.str()` seems redundant ================ Comment at: lib/Basic/VirtualFileSystem.cpp:804 + return this->addFile(FromPath, 0, nullptr, None, None, None, None, + static_cast<const detail::InMemoryFile *>(*ToNode)); +} ---------------- NIT: use `llvm::cast<InMemoryFile>` instead of static_cast. It's essentially equivalent with added asserts. ================ Comment at: unittests/Basic/VirtualFileSystemTest.cpp:974 +TEST_F(InMemoryFileSystemTest, AddHardLinkToFile) { + Twine FromLink = "/path/to/FROM/link"; + Twine Target = "/path/to/TO/file"; ---------------- Please use `StringRef`s everywhere. `Twine` should only be generally only be used as temporary objects, see the specific guidelines in Twine's docs. Whenever a reference to an existing string is needed, a `StringRef` is the best choice. If you need to create new strings (e.g. concatenation, etc.), use `std::string`. ================ Comment at: unittests/Basic/VirtualFileSystemTest.cpp:976 + Twine Target = "/path/to/TO/file"; + StringRef content = "content of target"; + FS.addFile(Target, 0, MemoryBuffer::getMemBuffer(content)); ---------------- s/content/Content ================ Comment at: unittests/Basic/VirtualFileSystemTest.cpp:987 + StringRef content = "content of target file"; + Twine link0 = "/path/to/0/link"; + Twine link1 = "/path/to/1/link"; ---------------- s/link0/Link0 .... and other `lowerCamelCase` vars --> `UpperCamelCase` ================ Comment at: unittests/Basic/VirtualFileSystemTest.cpp:1090 + } + EXPECT_EQ(1, Counts[0]); // a + EXPECT_EQ(1, Counts[1]); // b ---------------- This seems equivalent to `EXPECT_THAT(Nodes, UnorderedElementsAre("/a", "/a/b", "/c", "/c/d")`. Maybe simplify? Note that iterators might give different file separators on windows. You might want to normalize, e.g. with `llvm::sys::path::native(..., Style::posix)`. And still watch out for windows buildbot failures, in case the root (/) is interpreted differently on Windows, possibly giving different results. Repository: rC Clang https://reviews.llvm.org/D51359 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits