ioeric added a comment.

In, @ilya-biryukov wrote:

> In, @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!

  rC Clang

cfe-commits mailing list

Reply via email to