simark created this revision.
simark added reviewers: malaperle, ilya-biryukov, bkramer.
This is a new version of https://reviews.llvm.org/D48903, which has been
reverted. @ioeric fixed
the issues caused by this patch downstream, so he told me it was good to
go again. I also fixed the test failures on Windows. The issue is that
the paths returned by the InMemoryFileSystem directory iterators in the
tests mix posix and windows directory separators. This is because we do
queries with posix-style separators ("/a/b") but filenames are appended
using native-style separators (backslash on Windows). So we end up with
"/a/b\c".
I fixed the test by re-using the ReplaceBackslashes function defined in
another test. I'm not sure this is the best fix, but the only
alternative I see would be to completely rewrite tests to use
posix-style paths on non-Windows and Windows-style paths on Windows.
That would lead to quite a bit of duplication...
Here's the original commit message:
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.
Repository:
rC Clang
https://reviews.llvm.org/D49804
Files:
lib/Basic/FileManager.cpp
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
unittests/Driver/ToolChainTest.cpp
Index: unittests/Driver/ToolChainTest.cpp
===================================================================
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
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"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===================================================================
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -151,6 +151,11 @@
addEntry(Path, S);
}
};
+
+auto ReplaceBackslashes = [](std::string S) {
+ std::replace(S.begin(), S.end(), '\\', '/');
+ return S;
+};
} // end anonymous namespace
TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +787,7 @@
I = FS.dir_begin("/b", EC);
ASSERT_FALSE(EC);
- ASSERT_EQ("/b/c", I->getName());
+ ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
I.increment(EC);
ASSERT_FALSE(EC);
ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,16 +799,12 @@
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(
@@ -919,6 +920,37 @@
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);
+ ASSERT_EQ("../b/c", ReplaceBackslashes(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 {
Index: lib/Basic/VirtualFileSystem.cpp
===================================================================
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,31 @@
/// The in memory file system is a tree of Nodes. Every node can either be a
/// file or a directory.
class InMemoryNode {
- Status Stat;
InMemoryNodeKind Kind;
+ Status Stat;
+
+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) {}
+ : Kind(Kind), Stat(std::move(Stat)) {}
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,22 @@
}
};
-/// 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 +735,7 @@
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 +748,8 @@
// 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 +761,33 @@
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.str());
+ } 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 +803,8 @@
}
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>());
Index: lib/Basic/FileManager.cpp
===================================================================
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
