[PATCH] D54277: Extend VFS with function to get external path.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a subscriber: klimek.
sammccall added a comment.

Hi Jonas,

I'm not sure about this abstraction: because it's non-recursive, it's hard to give meaningful semantics to getExternalPath() - the thing that would be useful to LLDB (if abstraction-breaking) is "native OS path", but for a general VFS, getExternalPath may give a native path, fail, or give a string that's not a native OS path.
If dispatching between the real VFS and redirecting VFS in this way is the best thing for LLDB, I'd suggest exposing RedirectingFS and adding the indirection on the LLDB side.

I think *if* VFS is to support devirtualization, then putting it on the main VFS interface so it can be recursive as in your original patch is the right way to do it, and it needs to be explicitly optional. (i.e. supporting devirtualization is a dynamic property rather than a static one.)

For example:

  // If \p Path corresponds directly to a native operating system path, attempts to return that path.
  // (e.g. if this is the native file system, or merely remaps paths for an underlying native file system).
  // May return `None` if there is no such native file or its path cannot be determined.
  virtual Optional<std::string> getNativeOSPath(StringRef Path) { return None; }

(Note the lack of "error" return value: this makes it safe for VFSes that don't implement this to not distinguish between the "no file" and "can't be determined" case)

But I don't personally think this addition is a good idea at the VFS layer. The upside is it allows LLDB to use VFS in some places, which may eventually turn into true VFS support. The risk is that adding an escape hatch will mean over time we can no longer rely on VFS-aware code being compatible with all VFSes.

I think this could benefit from a discussion in the wider LLVM community - I'll start a discussion on llvm-dev.


https://reviews.llvm.org/D54277