dexonsmith added a comment.
This looks nice!
================
Comment at: clang/lib/Basic/FileManager.cpp:287-289
// name-as-accessed on the \a FileEntryRef. Maybe the returned \a
// FileEntryRef::getName() could return the accessed name unmodified, but
// make the external name available via a separate API.
----------------
I'd suggest updating this text to match the new FIXME next to `IsVFSMapped`
(instead of "maybe", list the plan).
================
Comment at: clang/lib/Basic/FileManager.cpp:316-318
+ // case above is removed. That one can be removed once we always return
+ // virtual paths and anywhere that needs external paths specifically
+ // requests them.
----------------
It's not obvious to me that the redirection case above depends on this logic
sticking around.
- If that's what you mean, can you explain in more detail why the redirection
above depends on this `UFE.Dir` logic?
- If you're just talking about `IsVFSMapped` having to stick around, I think
that part should be covered by the comment for the redirection.
================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:58-63
+ /// Whether the path in this status has been mapped by a VFS to another path.
bool IsVFSMapped = false;
+ // Note: Currently used by a hack in \c FileManager and internally in
+ // \c RedirectingFileSystem. We should change this to "HasVFSMapping" and
+ // *always* return the virtual path, only providing the mapped/external path
+ // when requested.
----------------
I think "mapped to another path" has a broader definition in my mind than
you're using here. IMO, all things in a redirecting filesystem map from one
path to another. The question is whether the mapping is leaked/available due to
use-external-names.
I think this comment be more precise, and the note should be a FIXME that's
part of the main comment, not something that comes later.
I also think it good to change the field name ~now (or in an immediately
following NFC patch) to reflect the new meaning. I like "exposes" or "leaks"
because that is clear that this indicates whether information about the mapping
is available / exposed in the abstraction, whereas "mapped" and "mapping" sound
to me like they're just talking about whether something was redirected. And I
like the word "external" because it ties back to use-external-names.
Suggested text (feel free to change entirely, but I think it covers a couple of
important points):
```
/// Whether this entity has an external path different from the virtual path,
and
/// the external path is exposed by leaking it through the abstraction. For
example,
/// a RedirectingFileSystem will set this for paths where UseExternalName is
true.
///
/// FIXME: Currently the external path is exposed by replacing the virtual path
/// in this Status object. Instead, we should leave the path in the Status
intact
/// (matching the requested virtual path), and just use this flag as hint that a
/// new API, FileSystem::getExternalVFSPath(), will not return `None`. Clients
can
/// then request the external path only where needed (e.g., for diagnostics, or
to
/// report discovered dependencies to external tools).
bool ExposesExternalVFSPath = false;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122549/new/
https://reviews.llvm.org/D122549
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
