[PATCH] D122549: [VFS] RedirectingFileSystem only replace path if not already mapped

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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