[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
Simon Marchi via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 5 14:29:23 PDT 2018
simark marked 6 inline comments as done.
simark added inline comments.
================
Comment at: lib/Basic/VirtualFileSystem.cpp:516
+ explicit InMemoryFileAdaptor(InMemoryFile &Node, 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> 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 &In, 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 &In, StringRef NewName) {
^
```
Repository:
rC Clang
https://reviews.llvm.org/D48903
More information about the cfe-commits
mailing list