dexonsmith accepted this revision.
dexonsmith added a reviewer: benlangmuir.
dexonsmith added a subscriber: benlangmuir.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM.

Once we've removed the last use of `DirectoryEntry::getName()`, we should drop 
`DirectoryEntry::getName()` to avoid picking up other users. Perhaps 
`DirectoryEntry` itself could be made private to the FileManager.

> The root cause is the fact that Clang is using DirectoryEntry::getName in the 
> header search algorithm. This function returns the path that was first used 
> to construct the (shared) entry in FileManager. Using 
> DirectoryEntryRef::getName instead preserves the case as it was spelled out 
> for the current "get directory entry" request.

For clarity, this is just one of various root causes of FileManager being 
unsound to reuse. Might be good to reword the commit message to avoid implying 
that it's the only one.

The obvious thing is that FileEntry is still used in a few places, especially 
in modules.

The non-obvious thing is that the "redirection hack" in 
`FileManager::getFileRef()`, which deals with a VFS remapping an external name, 
can cause future lookups to succeed that would previously have failed. 
@benlangmuir hit this recently when he was experimenting with NOT sharing the 
FileManager during implicit modules scanning; he can share the sequence of 
steps (I tried just now to remember them but I couldn't work it out).

IMO, we should turn off sharing the FileManager across multiple TUs (except as 
an experimental option) until we actually believe that it's sound. I'm not 
convinced there's a lot of benefit anyway:

- Stat failures need to be cleared between TUs
- Stat successes are cached by the dependency scanning filesystem
- The contents are (mostly? entirely?) bump-ptr allocated since 
https://reviews.llvm.org/D123144


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123229/new/

https://reviews.llvm.org/D123229

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to