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