[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
This revision was automatically updated to reflect the committed changes.
Closed by commit rC339063: [VirtualFileSystem] InMemoryFileSystem::status:
Return a Status with the… (authored by simark, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48903?vs=158625=159401#toc
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/FileManager.cpp
lib/Basic/VirtualFileSystem.cpp
test/PCH/case-insensitive-include.c
unittests/Basic/VirtualFileSystemTest.cpp
unittests/Driver/ToolChainTest.cpp
Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -13,7 +13,7 @@
// RUN: touch -r %t-dir/case-insensitive-include.h %t.copy
// RUN: mv %t.copy %t-dir/case-insensitive-include.h
-// RUN: %clang_cc1 -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
// expected-no-diagnostics
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
}
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
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 () const { return Stat; }
+
public:
InMemoryNode(Status Stat, InMemoryNodeKind Kind)
: Stat(std::move(Stat)), Kind(Kind) {}
virtual ~InMemoryNode() = default;
- const Status () 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 @@
}
};
-/// 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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
return Node.getError();
}
@@ -724,7 +747,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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,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);
+} else {
+ // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+ // do the rest.
+
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1189056, @ilya-biryukov wrote: > I see, thanks for the explanation. > > LGTM for the update revision, given we have confirmation the tests pass on > Windows. Thanks, I'll push it, let's hope this time is the right time! Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48903#1187605, @simark wrote: > In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote: > > > This revision got 'reopened' and is now in the list of accepted revision. > > Should we close it again? > > > It got reverted a second time because it was breaking a test on Windows. The > new bit is the change to `test/PCH/case-insensitive-include.c`, so it would > need review again. If somebody else could run the tests on Windows, it would > make me a bit more confident too. I see, thanks for the explanation. LGTM for the update revision, given we have confirmation the tests pass on Windows. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1188566, @malaperle wrote: > Both check-clang and check-clang-tools pass successfully for me on Windows > with the patch. Awesome, thanks! Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
malaperle added a comment. Both check-clang and check-clang-tools pass successfully for me on Windows with the patch. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
malaperle added a comment. In https://reviews.llvm.org/D48903#1188437, @malaperle wrote: > In https://reviews.llvm.org/D48903#1187605, @simark wrote: > > > If somebody else could run the tests on Windows, it would make me a bit > > more confident too. > > > Which tests/targets exactly? If you know NVM, I saw the "check-clang and check-clang-tools" above. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
malaperle added a comment. In https://reviews.llvm.org/D48903#1187605, @simark wrote: > If somebody else could run the tests on Windows, it would make me a bit more > confident too. Which tests/targets exactly? If you know Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark requested review of this revision. simark added a comment. In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote: > This revision got 'reopened' and is now in the list of accepted revision. > Should we close it again? It got reverted a second time because it was breaking a test on Windows. The new bit is the change to `test/PCH/case-insensitive-include.c`, so it would need review again. If somebody else could run the tests on Windows, it would make me a bit more confident too. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment. This revision got 'reopened' and is now in the list of accepted revision. Should we close it again? Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 158625.
simark added a comment.
Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/FileManager.cpp
lib/Basic/VirtualFileSystem.cpp
test/PCH/case-insensitive-include.c
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
@@ -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
@@ -151,6 +152,13 @@
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 @@
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 @@
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 @@
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 {
Index:
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
eric_niebler added a comment. > It seems to me like the warning is valid, even though we use precompiled > headers. Agreed. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a subscriber: eric_niebler. simark added a comment. @eric_niebler, question for you: This patch causes clang to emit a `-Wnonportable-include-path` warning where it did not before. It affects the following test on Windows: https://github.com/llvm-mirror/clang/blob/master/test/PCH/case-insensitive-include.c The warning is currently not emitted for the **last** clang invocation (the one that includes PCH), because the real path value is not available, and therefore this condition is false: https://github.com/llvm-mirror/clang/blob/fe1098c84823b8eac46b0bfffc5f5788b6c26d1a/lib/Lex/PPDirectives.cpp#L2015 With this patch, the real path value is available, so we emit the warning. Is it on purpose that this warning is not emitted in this case? If not should I simply add `-Wno-nonportable-include-path` to the last clang invocation, as was done earlier with the first invocation? It seems to me like the warning is valid, even though we use precompiled headers. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
This revision was automatically updated to reflect the committed changes.
Closed by commit rC338057: [VirtualFileSystem] InMemoryFileSystem::status:
Return a Status with the… (authored by simark, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48903?vs=157467=157548#toc
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/FileManager.cpp
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
unittests/Driver/ToolChainTest.cpp
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
}
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
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 () const { return Stat; }
+
public:
InMemoryNode(Status Stat, InMemoryNodeKind Kind)
: Stat(std::move(Stat)), Kind(Kind) {}
virtual ~InMemoryNode() = default;
- const Status () 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 @@
}
};
-/// 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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
return Node.getError();
}
@@ -724,7 +747,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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,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);
+} 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 )
- : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
- CurrentEntry = I->second->getStatus();
+ explicit InMemoryDirIterator(detail::InMemoryDirectory ,
+ std::string RequestedDirName)
+ : I(Dir.begin()), E(Dir.end()),
+RequestedDirName(std::move(RequestedDirName)) {
+setCurrentEntry();
}
std::error_code increment() override {
++I;
-// When we're at
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 157467.
simark marked an inline comment as done.
simark added a comment.
I think this addresses all of Ilya's comments.
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
@@ -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
@@ -151,6 +152,13 @@
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 @@
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 @@
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 @@
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 {
Index: lib/Basic/VirtualFileSystem.cpp
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark marked 7 inline comments as done. simark added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:475 InMemoryNodeKind Kind; + Status Stat; + ilya-biryukov wrote: > NIT: maybe keep the order of members the same to keep the patch more focused? > Unless there's a good reason to swap them. Oops, that was not intended. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment.
Many thanks! Great cleanup. Just a few nits and we're good to go
Comment at: lib/Basic/VirtualFileSystem.cpp:475
InMemoryNodeKind Kind;
+ Status Stat;
+
NIT: maybe keep the order of members the same to keep the patch more focused?
Unless there's a good reason to swap them.
Comment at: lib/Basic/VirtualFileSystem.cpp:528
InMemoryFile
+ /// The name to use when returning a Status for this file.
NIT: remove this blank line to follow the code style of the file more closely?
Comment at: lib/Basic/VirtualFileSystem.cpp:770
+ llvm::sys::path::append(Path, I->second->getFileName());
+ CurrentEntry = I->second->getStatus(Path.str());
+} else {
NIT: `Path.str()` can be replaced with `Path` (SmallString is convertible to
StringRef)
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:155
+
+auto ReplaceBackslashes = [](std::string S) {
+ std::replace(S.begin(), S.end(), '\\', '/');
Maybe replace lambda with a funciton?
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:155
+
+auto ReplaceBackslashes = [](std::string S) {
+ std::replace(S.begin(), S.end(), '\\', '/');
ilya-biryukov wrote:
> Maybe replace lambda with a funciton?
Could the name mention the expected result? E.g. `getUnixPath()` or something
similar
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:156
+auto ReplaceBackslashes = [](std::string S) {
+ std::replace(S.begin(), S.end(), '\\', '/');
+ return S;
Maybe use `llvm::sys::path::native(S, style::posix)`?
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:790
ASSERT_FALSE(EC);
- ASSERT_EQ("/b/c", I->getName());
+ ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
I.increment(EC);
Maybe add a comment about windows and the path we get there?
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 157287.
simark added a comment.
Fix tests on Windows
Fix InMemoryFileSystem tests on Windows. 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 filnames 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...
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark reopened this revision. simark added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48903#1175317, @ioeric wrote: > It would make it easier for your reviewers to look at the new changes if > you just reopen this patch and update the diff :) I tried but didn't know how. But I just saw the "Add Action" drop down with "Reopen" in it. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a subscriber: malaperle. ioeric added a comment. It would make it easier for your reviewers to look at the new changes if you just reopen this patch and update the diff :) Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
It would make it easier for your reviewers to look at the new changes if you just reopen this patch and update the diff :) On Wed, Jul 25, 2018 at 5:49 PM Simon Marchi via Phabricator < revi...@reviews.llvm.org> wrote: > simark added a comment. > > I uploaded a new version of this patch here: > https://reviews.llvm.org/D49804 > > > Repository: > rC Clang > > https://reviews.llvm.org/D48903 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. I uploaded a new version of this patch here: https://reviews.llvm.org/D49804 Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a comment. In https://reviews.llvm.org/D48903#1159985, @simark wrote: > In https://reviews.llvm.org/D48903#1159303, @ioeric wrote: > > > For example, suppose you have header search directories > > `-I/path/to/include` and `-I.` in your compile command. When preprocessor > > searches for an #include "x.h", it will try to stat "/path/to/include/x.h" > > and "./x.h" and take the first one that exists. This can introduce > > indeterminism for the path (./x.h or /abs/x.h) you later get for the header > > file, e.g. when you try to look up file name by `FileID` through > > `SourceManager`. The path you get for a `FileEntry` or `FileID` would > > depend on how clang looks up a file and how a file is first opened into > > `SourceManager`/`FileManager`. It seems that the current behavior of > > clangd + in-memory file system would give you paths that are relative to > > the working directory for both cases. I'm not sure if that's the best > > behavior, but I think the consistency has its value. For example, in unit > > tests where in-memory file systems are heavily used, it's important to have > > a way to compare the reported file path (e.g. file path corresponding to a > > source location) with the expected paths. We could choose to always return > > real path, which could be potentially expensive, or we could require users > > to always compare real paths (it's unclear how well this would work though > > e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the > > API. Otherwise, I worry it would cause more confusions for folks who use > > clang with in-memory file system in the future. > > > It's hard to tell without seeing an actual failing use case. I think the clang-move test you mitigated in https://reviews.llvm.org/D48951 is an example. When setting up compiler instance, it uses `-I.` in the compile command (https://github.com/llvm-mirror/clang-tools-extra/blob/master/unittests/clang-move/ClangMoveTests.cpp#L236), which results in the header paths that start with `./`. If you change replace `.` with the absolute path of the working directory e.g. `-I/usr/include`, the paths you get will start with `/usr/include/`. This is caused by the way preprocessor looks up include headers. We could require users (e.g. the clang-move test writer) to clean up dots, but this has to be enforced somehow by the framework. > I understand the argument, but I think the necessity to work as closely as > `RealFileSystem` as possible is important. Otherwise, it becomes impossible > to reproduce bugs happening in the real world in unittests. FWIW, I don't think we could reproduce all real world bugs with unit tests ;) But I agree with you that we should try to converge the behavior if possible, and I support the motivation of this change ;) This change would be perfectly fine as is if in-mem fs is always used directly by users, but the problem is that it's also used inside clang and clang tooling, where users don't control over how files are opened. My point is we should do this in a way that doesn't introduce inconsistency/confusion for users of clang and tooling framework. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1159303, @ioeric wrote: > For example, suppose you have header search directories `-I/path/to/include` > and `-I.` in your compile command. When preprocessor searches for an #include > "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the > first one that exists. This can introduce indeterminism for the path (./x.h > or /abs/x.h) you later get for the header file, e.g. when you try to look up > file name by `FileID` through `SourceManager`. The path you get for a > `FileEntry` or `FileID` would depend on how clang looks up a file and how a > file is first opened into `SourceManager`/`FileManager`. It seems that the > current behavior of clangd + in-memory file system would give you paths that > are relative to the working directory for both cases. I'm not sure if that's > the best behavior, but I think the consistency has its value. For example, in > unit tests where in-memory file systems are heavily used, it's important to > have a way to compare the reported file path (e.g. file path corresponding to > a source location) with the expected paths. We could choose to always return > real path, which could be potentially expensive, or we could require users to > always compare real paths (it's unclear how well this would work though e.g. > ClangTool doesn't expose its vfs), but they need to be enforced by the API. > Otherwise, I worry it would cause more confusions for folks who use clang > with in-memory file system in the future. It's hard to tell without seeing an actual failing use case. I understand the argument, but I think the necessity to work as closely as `RealFileSystem` as possible is important. Otherwise, it becomes impossible to reproduce bugs happening in the real world in unittests. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a comment. After looking at a few more use cases of the in-memory file system, I think a problem we need to address is the consistency of file paths you get from clang when using in-mem vfs. The clang-move tests that you have mitigated in https://reviews.llvm.org/D48951 could be an example. For example, suppose you have header search directories `-I/path/to/include` and `-I.` in your compile command. When preprocessor searches for an #include "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the first one that exists. This can introduce indeterminism for the path (./x.h or /abs/x.h) you later get for the header file, e.g. when you try to look up file name by `FileID` through `SourceManager`. The path you get for a `FileEntry` or `FileID` would depend on how clang looks up a file and how a file is first opened into `SourceManager`/`FileManager`. It seems that the current behavior of clangd + in-memory file system would give you paths that are relative to the working directory for both cases. I'm not sure if that's the best behavior, but I think the consistency has its value. For example, in unit tests where in-memory file systems are heavily used, it's important to have a way to compare the reported file path (e.g. file path corresponding to a source location) with the expected paths. We could choose to always return real path, which could be potentially expensive, or we could require users to always compare real paths (it's unclear how well this would work though e.g. ClangTool doesn't expose its vfs), but they need to be enforced by the API. Otherwise, I worry it would cause more confusions for folks who use clang with in-memory file system in the future. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a comment. In https://reviews.llvm.org/D48903#1159046, @simark wrote: > In https://reviews.llvm.org/D48903#1159023, @ioeric wrote: > > > Would you mind reverting this patch for now so that we can come up with a > > solution to address those use cases? > > > > Sorry again about missing the discussion earlier! > > > Of course, feel free to revert if needed (I'm not sure how to do that). Are > you able to come up with a test case that covers the use case you mention? Thanks! I'll try to come up with a smaller test case asap. The virtual file support in `FileManager` is totally confusing and full of traps (when used with VFS). I really feel we should replace it with InMemoryFileSystem at some point :( Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1159023, @ioeric wrote: > Would you mind reverting this patch for now so that we can come up with a > solution to address those use cases? > > Sorry again about missing the discussion earlier! Of course, feel free to revert if needed (I'm not sure how to do that). Are you able to come up with a test case that covers the use case you mention? Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a comment. In https://reviews.llvm.org/D48903#1159023, @ioeric wrote: > In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote: > > > In https://reviews.llvm.org/D48903#1154846, @simark wrote: > > > > > With the `InMemoryFileSystem`, this now returns a non-real path. The > > > result is that we fill `RealPathName` with that non-real path. I see two > > > options here: > > > > > > 1. Either the FileManager is wrong to assume that `File::getName` returns > > > a real path, and should call `FS->getRealPath` to do the job. > > > 2. If the contract is that the ` File::getName` interface should return a > > > real path, then we should fix the `File::getName` implementation to do > > > that somehow. > > > > > > I would opt for 1. This way, we could compute the `RealPathName` field > > > even if we don't open the file (unlike currently). > > > > > > I'd also say `FileManager` should use `FileSystem::getRealPath`. The code > > that does it differently was there before `FileSystem::getRealPath` was > > added. > > And `RealFile` should probably not do any magic in `getName`, we could add > > a separate method for (`getRealName`?) if that's absolutely needed. > > > > Refactorings in that area would probably break stuff and won't be trivial > > and I don't think this change should be blocked by those. So I'd be happy > > if this landed right away with a FIXME in `FileManager` mentioning that > > `InMemoryFileSystem` might give surprising results there. > > @ioeric added `FileSystem::getRealPath`, he may more ideas on how we > > should proceed. > > > Sorry for being late in the discussion. I'm not sure if `getRealPath` is the > right thing to do in `FileManager` as it also supports virtual files added > via `FileManager::getVirtualFile`, and they don't necessary have a real path. > This would also break users of `ClangTool::mapVirtualFile` when the mapped > files are relative. As a matter of fact, I'm already seeing related breakages > in our downstream branch :( > > Would you mind reverting this patch for now so that we can come up with a > solution to address those use cases? > > Sorry again about missing the discussion earlier! Sorry, having taken a closer look, it seems that only `ClangTool::mapVirtualFile` would be affected. I'll try to see if there is an easy fix. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ioeric added a comment. In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote: > In https://reviews.llvm.org/D48903#1154846, @simark wrote: > > > With the `InMemoryFileSystem`, this now returns a non-real path. The > > result is that we fill `RealPathName` with that non-real path. I see two > > options here: > > > > 1. Either the FileManager is wrong to assume that `File::getName` returns a > > real path, and should call `FS->getRealPath` to do the job. > > 2. If the contract is that the ` File::getName` interface should return a > > real path, then we should fix the `File::getName` implementation to do that > > somehow. > > > > I would opt for 1. This way, we could compute the `RealPathName` field > > even if we don't open the file (unlike currently). > > > I'd also say `FileManager` should use `FileSystem::getRealPath`. The code > that does it differently was there before `FileSystem::getRealPath` was added. > And `RealFile` should probably not do any magic in `getName`, we could add a > separate method for (`getRealName`?) if that's absolutely needed. > > Refactorings in that area would probably break stuff and won't be trivial and > I don't think this change should be blocked by those. So I'd be happy if this > landed right away with a FIXME in `FileManager` mentioning that > `InMemoryFileSystem` might give surprising results there. > @ioeric added `FileSystem::getRealPath`, he may more ideas on how we should > proceed. Sorry for being late in the discussion. I'm not sure if `getRealPath` is the right thing to do in `FileManager` as it also supports virtual files added via `FileManager::getVirtualFile`, and they don't necessary have a real path. This would also break users of `ClangTool::mapVirtualFile` when the mapped files are relative. As a matter of fact, I'm already seeing related breakages in our downstream branch :( Would you mind reverting this patch for now so that we can come up with a solution to address those use cases? Sorry again about missing the discussion earlier! Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
This revision was automatically updated to reflect the committed changes.
Closed by commit rC336807: [VirtualFileSystem] InMemoryFileSystem::status:
Return a Status with the… (authored by simark, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48903?vs=154835=154995#toc
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/FileManager.cpp
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
unittests/Driver/ToolChainTest.cpp
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
}
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,33 @@
/// 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 () 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 +522,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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +737,7 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
return Node.getError();
}
@@ -724,7 +750,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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +763,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 )
- : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
- CurrentEntry = I->second->getStatus();
+ explicit InMemoryDirIterator(detail::InMemoryDirectory ,
+ std::string
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment. In https://reviews.llvm.org/D48903#1157600, @simark wrote: > Thanks. I have seen no failures in `check-clang` and `check-clang-tools`, so > I will push it. LG! We can always revert the change is anything breaks... Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154835.
simark added a comment.
Bump SmallString size from 32 to 256
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,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", 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,33 @@
/// 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 () 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 +522,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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) :
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark marked an inline comment as done. simark added a comment. In https://reviews.llvm.org/D48903#1157330, @ilya-biryukov wrote: > LGTM if that does not introduce any regressions in clang and clang-tools. Thanks. I have seen no failures in `check-clang` and `check-clang-tools`, so I will push it. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM if that does not introduce any regressions in clang and clang-tools.
Comment at: lib/Basic/VirtualFileSystem.cpp:770
+if (I != E) {
+ SmallString<32> Path(RequestedDirName);
+ llvm::sys::path::append(Path, I->second->getFileName());
NIT: maybe increase the size to 256? This could save an extra allocation in
some cases, and hopefully won't be expensive, stack allocs are cheap in most
cases.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154782.
simark marked 5 inline comments as done.
simark added a comment.
- Make InMemoryNode::Stat private again, add protected accessor.
- Change condition formatting.
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,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", 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,33 @@
/// 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 () 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 +522,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
+ /// The name to use when returning a Status for this
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added inline comments.
Comment at: lib/Basic/FileManager.cpp:320
+ SmallString<128> RealPathName;
+ if (FS->getRealPath(InterndFileName, RealPathName) == std::error_code())
+UFE.RealPathName = RealPathName.str();
NIT: replace replace equality with negative test, i.e. `if
(!FS->getRealPath(…))`
I'm not a big fan of bash-like error codes, but that seems to be the idiomatic
way to use them.
Comment at: lib/Basic/VirtualFileSystem.cpp:488
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
simark wrote:
> ilya-biryukov wrote:
> > Given that this method is inconsistent with `getStatus()` and seems to be
> > only used in `toString` methods, maybe we could make it `protected`?
> > Otherwise it's really easy to write code that gets the wrong path.
> I now use it in `InMemoryDirIterator::setCurrentEntry`. I will write a
> comment to the `getName` method to clarify this.
`getFileName` as a public method and its usage in setCurrentEntry LG , thanks!
Comment at: lib/Basic/VirtualFileSystem.cpp:477
+protected:
+ Status Stat;
+
The inheritors should not be able to modify this field.
Can we get away with a private field and a protected getter that returns a
const reference instead?
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154662.
simark added a comment.
- Change InMemoryNode::getName to InMemoryNode::getFileName, to reduce the risk
of mis-using it. Make the Stat field protected, make the subclasses' toString
access it directly.
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,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", 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,27 @@
/// 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;
+protected:
+ Status Stat;
+
public:
InMemoryNode(Status Stat, InMemoryNodeKind Kind)
- : Stat(std::move(Stat)), Kind(Kind) {}
+ : Kind(Kind), Stat(std::move(Stat)) {}
virtual ~InMemoryNode() = default;
- const Status () 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;
};
@@ -496,22 +508,30 @@
llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + Stat.getName() + "\n").str();
}
static bool classof(const InMemoryNode *N) {
return N->getKind() == IME_File;
}
};
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface. The goal is to make
+/// \p InMemoryFileAdaptor mimic as
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154631.
simark added a comment.
- Use FileSystem::getRealPath in FileManager::getFile
Repository:
rC Clang
https://reviews.llvm.org/D48903
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,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", 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
@@ -479,7 +479,13 @@
: Stat(std::move(Stat)), Kind(Kind) {}
virtual ~InMemoryNode() = default;
- const Status () 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);
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
virtual std::string toString(unsigned Indent) const = 0;
};
@@ -496,22 +502,30 @@
llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
}
static bool classof(const InMemoryNode *N) {
return N->getKind() == IME_File;
}
};
-/// 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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+return
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark marked an inline comment as done. simark added a comment. In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote: > In https://reviews.llvm.org/D48903#1154846, @simark wrote: > > > With the `InMemoryFileSystem`, this now returns a non-real path. The > > result is that we fill `RealPathName` with that non-real path. I see two > > options here: > > > > 1. Either the FileManager is wrong to assume that `File::getName` returns a > > real path, and should call `FS->getRealPath` to do the job. > > 2. If the contract is that the ` File::getName` interface should return a > > real path, then we should fix the `File::getName` implementation to do that > > somehow. > > > > I would opt for 1. This way, we could compute the `RealPathName` field > > even if we don't open the file (unlike currently). > > > I'd also say `FileManager` should use `FileSystem::getRealPath`. The code > that does it differently was there before `FileSystem::getRealPath` was added. > And `RealFile` should probably not do any magic in `getName`, we could add a > separate method for (`getRealName`?) if that's absolutely needed. I made `FileManager::getFile` use `FileSystem::getRealPath` and see no regression in clang and clang-tools-extra. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark marked 6 inline comments as done.
simark added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:488
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
ilya-biryukov wrote:
> Given that this method is inconsistent with `getStatus()` and seems to be
> only used in `toString` methods, maybe we could make it `protected`?
> Otherwise it's really easy to write code that gets the wrong path.
I now use it in `InMemoryDirIterator::setCurrentEntry`. I will write a comment
to the `getName` method to clarify this.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:488
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
Given that this method is inconsistent with `getStatus()` and seems to be only
used in `toString` methods, maybe we could make it `protected`? Otherwise it's
really easy to write code that gets the wrong path.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment.
In https://reviews.llvm.org/D48903#1154846, @simark wrote:
> With the `InMemoryFileSystem`, this now returns a non-real path. The result
> is that we fill `RealPathName` with that non-real path. I see two options
> here:
>
> 1. Either the FileManager is wrong to assume that `File::getName` returns a
> real path, and should call `FS->getRealPath` to do the job.
> 2. If the contract is that the ` File::getName` interface should return a
> real path, then we should fix the `File::getName` implementation to do that
> somehow.
>
> I would opt for 1. This way, we could compute the `RealPathName` field
> even if we don't open the file (unlike currently).
I'd also say `FileManager` should use `FileSystem::getRealPath`. The code that
does it differently was there before `FileSystem::getRealPath` was added.
And `RealFile` should probably not do any magic in `getName`, we could add a
separate method for (`getRealName`?) if that's absolutely needed.
Refactorings in that area would probably break stuff and won't be trivial and I
don't think this change should be blocked by those. So I'd be happy if this
landed right away with a FIXME in `FileManager` mentioning that
`InMemoryFileSystem` might give surprising results there.
@ioeric added `FileSystem::getRealPath`, he may more ideas on how we should
proceed.
Comment at: lib/Basic/VirtualFileSystem.cpp:516
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
simark wrote:
> ilya-biryukov wrote:
> > NIT: The formatting is broken here.
> Hmm this is what git-clang-format does (unless this comment refers to a
> previous version where I had not run clang-format).
This LG now, it was unindented in the original version.
Comment at: lib/Basic/VirtualFileSystem.cpp:724
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
simark wrote:
> ilya-biryukov wrote:
> > NIT: we don't need `str()` here
> Otherwise I'm getting this:
>
> ```
> /home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:1673:9:
> error: no matching function for call to 'copyWithNewName'
> S = Status::copyWithNewName(S, Path);
> ^~~
> /home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:76:16:
> note: candidate function not viable: no known conversion from 'const
> llvm::Twine' to 'llvm::StringRef' for 2nd argument
> Status Status::copyWithNewName(const Status , StringRef NewName) {
>^
> /home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:82:16:
> note: candidate function not viable: no known conversion from
> 'clang::vfs::Status' to 'const llvm::sys::fs::file_status' for 1st argument
> Status Status::copyWithNewName(const file_status , StringRef NewName) {
>^
> ```
Sorry, I thought Path is `StringRef`, but it's actually `Twine`, so we do need
the str() call.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment.
I found something fishy. There is this code in FileManager.cpp:
if (UFE.File)
if (auto RealPathName = UFE.File->getName())
UFE.RealPathName = *RealPathName;
The real path is obtained from `UFE.File->getName()`. In the `RealFile`
implementation, it returns the `RealName` field, which is fine. For other
implementations, it uses `File::getName`, which is:
/// Get the name of the file
virtual llvm::ErrorOr getName() {
if (auto Status = status())
return Status->getName().str();
else
return Status.getError();
}
With the `InMemoryFileSystem`, this now returns a non-real path. The result is
that we fill `RealPathName` with that non-real path. I see two options here:
1. Either the FileManager is wrong to assume that `File::getName` returns a
real path, and should call `FS->getRealPath` to do the job.
2. If the contract is that the ` File::getName` interface should return a real
path, then we should fix the `File::getName` implementation to do that somehow.
I would opt for 1. This way, we could compute the `RealPathName` field even if
we don't open the file (unlike currently).
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154422.
simark marked an inline comment as done.
simark added a comment.
- Use StringRef in InMemoryNode::getStatus
- Remove unused variable in TEST_F(InMemoryFileSystemTest, StatusName)
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,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", 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
@@ -479,7 +479,13 @@
: Stat(std::move(Stat)), Kind(Kind) {}
virtual ~InMemoryNode() = default;
- const Status () 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);
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
virtual std::string toString(unsigned Indent) const = 0;
};
@@ -496,22 +502,30 @@
llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
}
static bool classof(const InMemoryNode *N) {
return N->getKind() == IME_File;
}
};
-/// 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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
hokein added a comment.
Herald added a subscriber: omtcyfz.
The code looks good. I'll let Ben take a final look.
Comment at: lib/Basic/VirtualFileSystem.cpp:485
+ /// \p Status::Name in the return value, to mimic the behavior of \p
RealFile.
+ Status getStatus(std::string RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
Can we use llvm::StringRef here?
Comment at: unittests/Basic/VirtualFileSystemTest.cpp:950
+ clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+ clang::vfs::directory_iterator End;
+
This is not used.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154321.
simark marked 4 inline comments as done.
simark added a comment.
- Add RequestedName to InMemoryNode::getStatus.
- Also fix the directory_iterator code path.
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
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
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,39 @@
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);
+ clang::vfs::directory_iterator End;
+
+ ASSERT_EQ("../b/c", 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
@@ -479,7 +479,13 @@
: Stat(std::move(Stat)), Kind(Kind) {}
virtual ~InMemoryNode() = default;
- const Status () 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(std::string RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+ }
+ StringRef getName() const { return Stat.getName(); }
InMemoryNodeKind getKind() const { return Kind; }
virtual std::string toString(unsigned Indent) const = 0;
};
@@ -496,22 +502,30 @@
llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
}
static bool classof(const InMemoryNode *N) {
return N->getKind() == IME_File;
}
};
-/// 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
+ /// The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override {
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark marked 6 inline comments as done.
simark added inline comments.
Comment at: lib/Basic/VirtualFileSystem.cpp:516
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
ilya-biryukov wrote:
> NIT: The formatting is broken here.
Hmm this is what git-clang-format does (unless this comment refers to a
previous version where I had not run clang-format).
Comment at: lib/Basic/VirtualFileSystem.cpp:520
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
ilya-biryukov wrote:
> Maybe add a `RequestedName` parameter to the `InMemoryNode` instead to make
> sure it's not misused?
> It looks like all the clients calling it have to change the name and some are
> not doing it now, e.g. the directory iterator will use statuses with full
> paths.
Ok, I tried to do something like that.
Comment at: lib/Basic/VirtualFileSystem.cpp:521
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
ilya-biryukov wrote:
> Maybe add a comment that this matches the real file system behavior?
I added a comment to `InMemoryNode::getSatus`, since that's where the
`copyWithNewName` is done now.
Comment at: lib/Basic/VirtualFileSystem.cpp:724
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
ilya-biryukov wrote:
> NIT: we don't need `str()` here
Otherwise I'm getting this:
```
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:1673:9:
error: no matching function for call to 'copyWithNewName'
S = Status::copyWithNewName(S, Path);
^~~
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:76:16: note:
candidate function not viable: no known conversion from 'const llvm::Twine' to
'llvm::StringRef' for 2nd argument
Status Status::copyWithNewName(const Status , StringRef NewName) {
^
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:82:16: note:
candidate function not viable: no known conversion from 'clang::vfs::Status' to
'const llvm::sys::fs::file_status' for 1st argument
Status Status::copyWithNewName(const file_status , StringRef NewName) {
^
```
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1153142, @hokein wrote: > Seems to me you have a few comments unaddressed (and make sure you marked > them done when updating the patch). Ah damn I missed them, I'm not too used to how Phabricator displays things. I'll do that. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
hokein added a comment. Seems to me you have a few comments unaddressed (and make sure you marked them done when updating the patch). Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. I opened https://reviews.llvm.org/D48951 to fix the failures in clang-move. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1152485, @ilya-biryukov wrote: > I usually run `ninja check-clang check-clang-tools` for clang changes. Have > never used `clang-test`, not sure what it does. I think `clang-test` is an alias for `check-clang`. > I ran it with this change, found a few failures from clang-move: > > Failing Tests (5): > Extra Tools Unit Tests :: > clang-move/./ClangMoveTests/ClangMove.AddDependentNewHeader > Extra Tools Unit Tests :: > clang-move/./ClangMoveTests/ClangMove.AddDependentOldHeader > Extra Tools Unit Tests :: > clang-move/./ClangMoveTests/ClangMove.DontMoveAll > Extra Tools Unit Tests :: > clang-move/./ClangMoveTests/ClangMove.MoveHeaderAndCC > Extra Tools Unit Tests :: > clang-move/./ClangMoveTests/ClangMove.MoveHeaderOnly Doh, I'll run `check-clang-tools` too. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment. I usually run `ninja check-clang check-clang-tools` for clang changes. Have never used `clang-test`, not sure what it does. I ran it with this change, found a few failures from clang-move: Failing Tests (5): Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentNewHeader Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.AddDependentOldHeader Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.DontMoveAll Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderAndCC Extra Tools Unit Tests :: clang-move/./ClangMoveTests/ClangMove.MoveHeaderOnly Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. I ran `ninja clang-test`: Testing Time: 1720.20s Expected Passes: 12472 Expected Failures : 18 Unsupported Tests : 263 Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154118.
simark added a comment.
- Fixed formatting (ran git-clang-format)
- Fixed expectation in TEST_F(InMemoryFileSystemTest, WorkingDirectory)
- Added test TEST_F(InMemoryFileSystemTest, StatusName)
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/VirtualFileSystem.cpp
unittests/Basic/VirtualFileSystemTest.cpp
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,30 @@
ASSERT_TRUE(Stat->isRegularFile());
}
+// Test that the name returned by status() is in the same form as the path that
+// was requested.
+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");
+
+ 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());
+
+ 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());
+}
+
// 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
@@ -508,10 +508,17 @@
class InMemoryFileAdaptor : public File {
InMemoryFile
+ // The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName(std::move(RequestedName)) {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -710,8 +717,10 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
- if (Node)
-return (*Node)->getStatus();
+ if (Node) {
+Status St = (*Node)->getStatus();
+return Status::copyWithNewName(St, Path.str());
+ }
return Node.getError();
}
@@ -724,7 +733,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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
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");
@@ -919,6 +919,30 @@
ASSERT_TRUE(Stat->isRegularFile());
}
+// Test that the name returned by status() is in the same form as the path that
+// was requested.
+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");
+
+ auto Stat = NormalizedFS.status("../b/c");
+ ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+ ASSERT_TRUE(Stat->isRegularFile());
+ ASSERT_EQ("../b/c",
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1151866, @ilya-biryukov wrote: > Mimicing RealFS seems like the right thing to do here, so I would vouch for > checking this change in. > I'm a little worried that there are tests/clients relying on the old > behavior, have you run all the tests? I didn't have time yesterday, I'm doing this now. Is `ninja/make clang-test` sufficient? Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark added a comment. In https://reviews.llvm.org/D48903#1151847, @hokein wrote: > I'm not familiar with this part of code, but the change looks fine to me. I > think @bkramer is the right person to review it. > > Please make sure the style align with LLVM code style. Woops indeed I forgot to run `git clang-format`. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a comment. Sorry, have missed the @hokein 's comments, so one of mine seems like a duplicate. Repository: rC Clang https://reviews.llvm.org/D48903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
ilya-biryukov added a reviewer: ilya-biryukov.
ilya-biryukov added a comment.
Mimicing RealFS seems like the right thing to do here, so I would vouch for
checking this change in.
I'm a little worried that there are tests/clients relying on the old behavior,
have you run all the tests?
Also, could you please run `git-clang-format` to fix the formatting issues?
Comment at: lib/Basic/VirtualFileSystem.cpp:516
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
NIT: The formatting is broken here.
Comment at: lib/Basic/VirtualFileSystem.cpp:520
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
Maybe add a `RequestedName` parameter to the `InMemoryNode` instead to make
sure it's not misused?
It looks like all the clients calling it have to change the name and some are
not doing it now, e.g. the directory iterator will use statuses with full paths.
Comment at: lib/Basic/VirtualFileSystem.cpp:521
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
Maybe add a comment that this matches the real file system behavior?
Comment at: lib/Basic/VirtualFileSystem.cpp:722
if (Node)
-return (*Node)->getStatus();
+{
+ Status St = (*Node)->getStatus();
NIT: The indent is incorrect here.
Comment at: lib/Basic/VirtualFileSystem.cpp:724
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
NIT: we don't need `str()` here
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
hokein added subscribers: bkramer, hokein.
hokein added a comment.
I'm not familiar with this part of code, but the change looks fine to me. I
think @bkramer is the right person to review it.
Please make sure the style align with LLVM code style.
Comment at: lib/Basic/VirtualFileSystem.cpp:507
/// Adapt a InMemoryFile for VFS' File interface.
class InMemoryFileAdaptor : public File {
I think we should have a comment saying the InMemoryFile has the same behavior
as the real file system.
Repository:
rC Clang
https://reviews.llvm.org/D48903
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark updated this revision to Diff 154010.
simark added a comment.
Update commit message
Repository:
rC Clang
https://reviews.llvm.org/D48903
Files:
lib/Basic/VirtualFileSystem.cpp
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -508,10 +508,18 @@
class InMemoryFileAdaptor : public File {
InMemoryFile
+ // The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +719,10 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+{
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
return Node.getError();
}
@@ -724,7 +735,7 @@
// 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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(new detail::InMemoryFileAdaptor(*F,
Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -508,10 +508,18 @@
class InMemoryFileAdaptor : public File {
InMemoryFile
+ // The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +719,10 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+{
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
return Node.getError();
}
@@ -724,7 +735,7 @@
// 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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
simark created this revision.
Herald added subscribers: cfe-commits, ioeric, ilya-biryukov.
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/D48903
Files:
lib/Basic/VirtualFileSystem.cpp
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -508,10 +508,18 @@
class InMemoryFileAdaptor : public File {
InMemoryFile
+ // The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +719,10 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+{
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
return Node.getError();
}
@@ -724,7 +735,7 @@
// 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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(new detail::InMemoryFileAdaptor(*F,
Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -508,10 +508,18 @@
class InMemoryFileAdaptor : public File {
InMemoryFile
+ // The name to use when returning a Status for this file.
+ std::string RequestedName;
+
public:
- explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+ explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+ : Node(Node), RequestedName (std::move (RequestedName))
+ {}
- llvm::ErrorOr status() override { return Node.getStatus(); }
+ llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+ }
llvm::ErrorOr>
getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +719,10 @@
llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
auto Node = lookupInMemoryNode(*this, Root.get(), Path);
if (Node)
-return (*Node)->getStatus();
+{
+ Status St = (*Node)->getStatus();
+ return Status::copyWithNewName(St, Path.str());
+}
return Node.getError();
}
@@ -724,7 +735,7 @@
// 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(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(new detail::InMemoryFileAdaptor(*F, Path.str()));
// FIXME: errc::not_a_file?
return make_error_code(llvm::errc::invalid_argument);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
