[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 9 01:06:22 PDT 2018
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 &Node, 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 &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) {
> ^
> ```
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
More information about the cfe-commits
mailing list