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
