[PATCH] D54277: Move RedirectingFileSystem interface into header.

[Kristina Brooks via Phabricator via llvm-commits]

kristina added a comment.

I'm not sure if this is way out of scope for this patch, but if I understand correctly this could be used for extending it to, for example, handle things like Perforce-style paths which could indeed be useful, although I'm not sure if that is possible (ie. would `//depot/asdf` behave differently on Windows and Linux/Darwin/any other OS that uses Unix-style paths?). If this is with the intention to allow extending `RedirectingFileSystem` downstream then I think it may be worth splitting it up from the YAML based implementation altogether, allowing custom path handling where needed (as a separate downstream extension), with the concrete reference implementation of `RedirectingFileSystem` (the YAML based one) being a separate thing. This would make it more extensible from the POV of downstream forks that may desire to implement path redirection that is too complicated to do using YAML mappings, but still fundamentally rely on some sort of path remapping.

Do you think it would be worth decoupling `RedirectingFileSystem` from YAML for such use cases and exposing both `RedirectingFileSystem` and, for your use case, something like `YAMLRedirectingFileSystem`? Just a stray thought, not really sure if it makes sense or is out of scope here (I don't believe it is, since if the interface is being exposed, it may be worth rethinking how it is layered if it's to be exposed through LLVMSupport headers).


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

https://reviews.llvm.org/D54277