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

[Ben Barham via Phabricator via llvm-commits]

bnbarham added a comment.

`clang-apply-replacements/relative-paths.cpp` is failing, I haven't looked into it but my guess would be that it's from the `Status.getName() == Filename` -> `!Status.IsVFSMapped` change. That seems very odd to me.



================
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.
----------------
dexonsmith wrote:
> I'd suggest updating this text to match the new FIXME next to `IsVFSMapped` (instead of "maybe", list the plan).
Will do


================
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.
----------------
dexonsmith wrote:
> 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.
I actually meant the reverse - the UFE.Dir logic being removed depends on the redirection logic above being removed. That's because for this to be removed, anywhere that cares about the requested path needs to be changed to use Refs. But even if they change to use Refs, the redirection logic above would itself replace the path with the external one. So it needs to be removed as well.


================
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.
----------------
dexonsmith wrote:
> 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;
> ```
> 
> 
That's fair. I suppose I was thinking that without `use-external-names: true` then nothing should be aware that it is "mapped" and thus "mapped" == "external". But I can see how that could be confusing.

I also thought that renaming now would be a little odd since eg. "HasExternalPath" seems a little different to "the actual path is currently the external path" (which is what it is right now). But with the "exposes" naming I think this could still make sense (especially with the FIXME you have). So I'll rename 👍 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122549