[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

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