Page MenuHomePhabricator

[VFS] Add "expand tilde" argument to getRealPath.
AbandonedPublic

Authored by JDevlieghere on Nov 12 2018, 10:57 AM.

Details

Summary

Add an optional argument to expand tildes in the path to mirror llvm's
implementation of the corresponding function.

Apparently this change is more controversial than I thought, so happy
to discuss alternatives here.

Diff Detail

Event Timeline

JDevlieghere created this revision.Nov 12 2018, 10:57 AM

I think this change was landed without review, and it's a complicated interface change, so I'm going to revert it for now.

I also have some specific concerns about the design:

  • this doesn't seem to be a per-filesystem concern. e.g. in an overlaid filesystem, whose responsibility is it to resolve ~? It's not clear this belongs in the virtualization layer at all.

I'm not sure this is true. It the overlay specifies a different home directory (this functionality doesn't exist yet) it should take that one, otherwise it should defer to the underlying one.

  • implementing this is a substantial burden on VFSes, many of which (by their nature) live out-of-tree. It's not clear what the consequences of ignoring the parameter are, and what callers can expect.
  • use of optional-args in virtual methods would be nice to avoid
  • the use of boolean params like this in general doesn't scale well

Would a separate method address those 3 concerns? My goal was to keep things simple and mirror llvm's API, which helps with migrating between the two. I don't have a strong opinion on this though, as long as the functionality is available.

Happy to help with review if you do want to move forward with this!

Thanks, I look forward to your input!

Thanks for sending this!
I think fs::real_path isn't a great precedent to be following here, but
that we can put the same functionality behind a slightly different API.

I may well be missing requirements, details, or historical context here.
Hopefully you or someone can correct me if so!

I created D54448 to add an expand_tilde function to llvm's filesystem as per Sam's suggestion. This still won't work for my reproducer feature in LLDB, but if everyone feels this doesn't belong in the VFS I can simply implement it there.

JDevlieghere abandoned this revision.Nov 15 2018, 10:36 AM