r338057 - [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
Reid Kleckner via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 26 16:22:16 PDT 2018
I reverted this in r338084 because it broke clang tests on Windows:
http://lab.llvm.org:8011/builders/clang-x86-windows-msvc2015/builds/12916
On Thu, Jul 26, 2018 at 11:55 AM Simon Marchi via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: simark
> Date: Thu Jul 26 11:55:02 2018
> New Revision: 338057
>
> URL: http://llvm.org/viewvc/llvm-project?rev=338057&view=rev
> Log:
> [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the
> requested name
>
> Summary:
>
> InMemoryFileSystem::status behaves differently than
> RealFileSystem::status. The Name contained in the Status returned by
> RealFileSystem::status will be the path as requested by the caller,
> whereas InMemoryFileSystem::status returns the normalized path.
>
> For example, when requested the status for "../src/first.h",
> RealFileSystem returns a Status with "../src/first.h" as the Name.
> InMemoryFileSystem returns "/absolute/path/to/src/first.h".
>
> The reason for this change is that I want to make a unit test in the
> clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
> bug I get with the clangd program (where a RealFileSystem is used).
> This difference in behavior "hides" the bug in the unit test version.
>
> Reviewers: malaperle, ilya-biryukov, bkramer
>
> Subscribers: cfe-commits, ioeric, ilya-biryukov, bkramer, hokein, omtcyfz
>
> Differential Revision: https://reviews.llvm.org/D48903
>
> Modified:
> cfe/trunk/lib/Basic/FileManager.cpp
> cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> cfe/trunk/unittests/Driver/ToolChainTest.cpp
>
> Modified: cfe/trunk/lib/Basic/FileManager.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=338057&r1=338056&r2=338057&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Basic/FileManager.cpp (original)
> +++ cfe/trunk/lib/Basic/FileManager.cpp Thu Jul 26 11:55:02 2018
> @@ -315,9 +315,11 @@ const FileEntry *FileManager::getFile(St
> UFE.InPCH = Data.InPCH;
> UFE.File = std::move(F);
> UFE.IsValid = true;
> - if (UFE.File)
> - if (auto RealPathName = UFE.File->getName())
> - UFE.RealPathName = *RealPathName;
> +
> + SmallString<128> RealPathName;
> + if (!FS->getRealPath(InterndFileName, RealPathName))
> + UFE.RealPathName = RealPathName.str();
> +
> return &UFE;
> }
>
>
> Modified: cfe/trunk/lib/Basic/VirtualFileSystem.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/VirtualFileSystem.cpp?rev=338057&r1=338056&r2=338057&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Basic/VirtualFileSystem.cpp (original)
> +++ cfe/trunk/lib/Basic/VirtualFileSystem.cpp Thu Jul 26 11:55:02 2018
> @@ -474,12 +474,28 @@ class InMemoryNode {
> Status Stat;
> InMemoryNodeKind Kind;
>
> +protected:
> + /// Return Stat. This should only be used for internal/debugging use.
> When
> + /// clients wants the Status of this node, they should use
> + /// \p getStatus(StringRef).
> + const Status &getStatus() const { return Stat; }
> +
> public:
> InMemoryNode(Status Stat, InMemoryNodeKind Kind)
> : Stat(std::move(Stat)), Kind(Kind) {}
> virtual ~InMemoryNode() = default;
>
> - const Status &getStatus() const { return Stat; }
> + /// Return the \p Status for this node. \p RequestedName should be the
> name
> + /// through which the caller referred to this node. It will override
> + /// \p Status::Name in the return value, to mimic the behavior of \p
> RealFile.
> + Status getStatus(StringRef RequestedName) const {
> + return Status::copyWithNewName(Stat, RequestedName);
> + }
> +
> + /// Get the filename of this node (the name without the directory part).
> + StringRef getFileName() const {
> + return llvm::sys::path::filename(Stat.getName());
> + }
> InMemoryNodeKind getKind() const { return Kind; }
> virtual std::string toString(unsigned Indent) const = 0;
> };
> @@ -504,14 +520,21 @@ public:
> }
> };
>
> -/// Adapt a InMemoryFile for VFS' File interface.
> +/// Adapt a InMemoryFile for VFS' File interface. The goal is to make
> +/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
> +/// \p RealFile.
> class InMemoryFileAdaptor : public File {
> InMemoryFile &Node;
> + /// The name to use when returning a Status for this file.
> + std::string RequestedName;
>
> public:
> - explicit InMemoryFileAdaptor(InMemoryFile &Node) : Node(Node) {}
> + explicit InMemoryFileAdaptor(InMemoryFile &Node, std::string
> RequestedName)
> + : Node(Node), RequestedName(std::move(RequestedName)) {}
>
> - llvm::ErrorOr<Status> status() override { return Node.getStatus(); }
> + llvm::ErrorOr<Status> status() override {
> + return Node.getStatus(RequestedName);
> + }
>
> llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
> getBuffer(const Twine &Name, int64_t FileSize, bool
> RequiresNullTerminator,
> @@ -711,7 +734,7 @@ lookupInMemoryNode(const InMemoryFileSys
> llvm::ErrorOr<Status> InMemoryFileSystem::status(const Twine &Path) {
> auto Node = lookupInMemoryNode(*this, Root.get(), Path);
> if (Node)
> - return (*Node)->getStatus();
> + return (*Node)->getStatus(Path.str());
> return Node.getError();
> }
>
> @@ -724,7 +747,8 @@ InMemoryFileSystem::openFileForRead(cons
> // When we have a file provide a heap-allocated wrapper for the memory
> buffer
> // to match the ownership semantics for File.
> if (auto *F = dyn_cast<detail::InMemoryFile>(*Node))
> - return std::unique_ptr<File>(new detail::InMemoryFileAdaptor(*F));
> + return std::unique_ptr<File>(
> + new detail::InMemoryFileAdaptor(*F, Path.str()));
>
> // FIXME: errc::not_a_file?
> return make_error_code(llvm::errc::invalid_argument);
> @@ -736,21 +760,33 @@ namespace {
> class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
> detail::InMemoryDirectory::const_iterator I;
> detail::InMemoryDirectory::const_iterator E;
> + std::string RequestedDirName;
> +
> + void setCurrentEntry() {
> + if (I != E) {
> + SmallString<256> Path(RequestedDirName);
> + llvm::sys::path::append(Path, I->second->getFileName());
> + CurrentEntry = I->second->getStatus(Path);
> + } else {
> + // When we're at the end, make CurrentEntry invalid and DirIterImpl
> will
> + // do the rest.
> + CurrentEntry = Status();
> + }
> + }
>
> public:
> InMemoryDirIterator() = default;
>
> - explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir)
> - : I(Dir.begin()), E(Dir.end()) {
> - if (I != E)
> - CurrentEntry = I->second->getStatus();
> + explicit InMemoryDirIterator(detail::InMemoryDirectory &Dir,
> + std::string RequestedDirName)
> + : I(Dir.begin()), E(Dir.end()),
> + RequestedDirName(std::move(RequestedDirName)) {
> + setCurrentEntry();
> }
>
> std::error_code increment() override {
> ++I;
> - // When we're at the end, make CurrentEntry invalid and DirIterImpl
> will do
> - // the rest.
> - CurrentEntry = I != E ? I->second->getStatus() : Status();
> + setCurrentEntry();
> return {};
> }
> };
> @@ -766,7 +802,8 @@ directory_iterator InMemoryFileSystem::d
> }
>
> if (auto *DirNode = dyn_cast<detail::InMemoryDirectory>(*Node))
> - return
> directory_iterator(std::make_shared<InMemoryDirIterator>(*DirNode));
> + return directory_iterator(
> + std::make_shared<InMemoryDirIterator>(*DirNode, Dir.str()));
>
> EC = make_error_code(llvm::errc::not_a_directory);
> return directory_iterator(std::make_shared<InMemoryDirIterator>());
>
> Modified: cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp?rev=338057&r1=338056&r2=338057&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp (original)
> +++ cfe/trunk/unittests/Basic/VirtualFileSystemTest.cpp Thu Jul 26
> 11:55:02 2018
> @@ -13,6 +13,7 @@
> #include "llvm/Support/Errc.h"
> #include "llvm/Support/Host.h"
> #include "llvm/Support/MemoryBuffer.h"
> +#include "llvm/Support/Path.h"
> #include "llvm/Support/SourceMgr.h"
> #include "gtest/gtest.h"
> #include <map>
> @@ -151,6 +152,13 @@ public:
> addEntry(Path, S);
> }
> };
> +
> +/// Replace back-slashes by front-slashes.
> +std::string getPosixPath(std::string S) {
> + SmallString<128> Result;
> + llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
> + return Result.str();
> +};
> } // end anonymous namespace
>
> TEST(VirtualFileSystemTest, StatusQueries) {
> @@ -782,7 +790,9 @@ TEST_F(InMemoryFileSystemTest, Directory
>
> I = FS.dir_begin("/b", EC);
> ASSERT_FALSE(EC);
> - ASSERT_EQ("/b/c", I->getName());
> + // When on Windows, we end up with "/b\\c" as the name. Convert to
> Posix
> + // path for the sake of the comparison.
> + ASSERT_EQ("/b/c", getPosixPath(I->getName()));
> I.increment(EC);
> ASSERT_FALSE(EC);
> ASSERT_EQ(vfs::directory_iterator(), I);
> @@ -794,23 +804,19 @@ TEST_F(InMemoryFileSystemTest, WorkingDi
>
> auto Stat = FS.status("/b/c");
> ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" <<
> FS.toString();
> - ASSERT_EQ("c", Stat->getName());
> + ASSERT_EQ("/b/c", Stat->getName());
> ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
>
> Stat = FS.status("c");
> ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" <<
> FS.toString();
>
> - auto ReplaceBackslashes = [](std::string S) {
> - std::replace(S.begin(), S.end(), '\\', '/');
> - return S;
> - };
> NormalizedFS.setCurrentWorkingDirectory("/b/c");
> NormalizedFS.setCurrentWorkingDirectory(".");
> - ASSERT_EQ("/b/c", ReplaceBackslashes(
> - NormalizedFS.getCurrentWorkingDirectory().get()));
> + ASSERT_EQ("/b/c",
> +
> getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
> NormalizedFS.setCurrentWorkingDirectory("..");
> - ASSERT_EQ("/b", ReplaceBackslashes(
> - NormalizedFS.getCurrentWorkingDirectory().get()));
> + ASSERT_EQ("/b",
> +
> getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
> }
>
> #if !defined(_WIN32)
> @@ -919,6 +925,39 @@ TEST_F(InMemoryFileSystemTest, AddDirect
> ASSERT_TRUE(Stat->isRegularFile());
> }
>
> +// Test that the name returned by status() is in the same form as the
> path that
> +// was requested (to match the behavior of RealFileSystem).
> +TEST_F(InMemoryFileSystemTest, StatusName) {
> + NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
> + /*User=*/None,
> + /*Group=*/None, sys::fs::file_type::regular_file);
> + NormalizedFS.setCurrentWorkingDirectory("/a/b");
> +
> + // Access using InMemoryFileSystem::status.
> + auto Stat = NormalizedFS.status("../b/c");
> + ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
> + << NormalizedFS.toString();
> + ASSERT_TRUE(Stat->isRegularFile());
> + ASSERT_EQ("../b/c", Stat->getName());
> +
> + // Access using InMemoryFileAdaptor::status.
> + auto File = NormalizedFS.openFileForRead("../b/c");
> + ASSERT_FALSE(File.getError()) << File.getError() << "\n"
> + << NormalizedFS.toString();
> + Stat = (*File)->status();
> + ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
> + << NormalizedFS.toString();
> + ASSERT_TRUE(Stat->isRegularFile());
> + ASSERT_EQ("../b/c", Stat->getName());
> +
> + // Access using a directory iterator.
> + std::error_code EC;
> + clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
> + // When on Windows, we end up with "../b\\c" as the name. Convert to
> Posix
> + // path for the sake of the comparison.
> + ASSERT_EQ("../b/c", getPosixPath(It->getName()));
> +}
> +
> // NOTE: in the tests below, we use '//root/' as our root directory,
> since it is
> // a legal *absolute* path on Windows as well as *nix.
> class VFSFromYAMLTest : public ::testing::Test {
>
> Modified: cfe/trunk/unittests/Driver/ToolChainTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Driver/ToolChainTest.cpp?rev=338057&r1=338056&r2=338057&view=diff
>
> ==============================================================================
> --- cfe/trunk/unittests/Driver/ToolChainTest.cpp (original)
> +++ cfe/trunk/unittests/Driver/ToolChainTest.cpp Thu Jul 26 11:55:02 2018
> @@ -113,7 +113,7 @@ TEST(ToolChainTest, VFSGCCInstallationRe
> std::replace(S.begin(), S.end(), '\\', '/');
> #endif
> EXPECT_EQ("Found candidate GCC installation: "
> - "/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
> + "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
> "Selected GCC installation: "
> "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
> "Candidate multilib: .;@m32\n"
>
>
> _______________________________________________
> 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/20180726/8fe5ecd5/attachment-0001.html>
More information about the cfe-commits
mailing list