Page MenuHomePhabricator

Added `Follow` parameter to llvm::vfs::FileSystem::status()
Needs ReviewPublic

Authored by alexfh on Feb 23 2021, 7:00 AM.

Details

Reviewers
sammccall
Summary

Currently, status() calls always resolve symlinks. This patch adds a parameter
to control this behavior.

Diff Detail

Event Timeline

alexfh created this revision.Feb 23 2021, 7:00 AM
alexfh requested review of this revision.Feb 23 2021, 7:00 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 23 2021, 7:00 AM

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

How strong is the need for this?
This adds complexity to a widely implemented and used interface, and the combination of virtual + default parameters can be at least a little confusing.

That's a fair question. Indeed, this is needed quire rarely, and I just removed what would be a use case for from our internal code. Even Status::isSymlink is almost exclusively used in tests. The main motivation for the patch is to make the FileSystem::status() API more aligned with sys::fs::status(), and allow Status::isSymlink to fulfil its purpose for Status instances coming from FileSystem::status(). Also a way to avoid confusion in the future. Definitely not a practical need right now.

If we do add this, I'd suggest a separate function rather than modifying the current one. I also think implementing it should be optional. Something like:

virtual ErrorOr<vfs::Status> status(const Twine &Path) = 0;
virtual ErrorOr<vfs::Status> status(const Twine &Path, bool FollowSymlinks) {
  // By default ignore the request not to follow symlinks.
  (void)FollowSymlinks;
  return status(Path);
}

or, maybe better / more clear with a distinct name:

virtual ErrorOr<vfs::Status> statusNoFollow(const Twine &Path) {
  // By default ignore the request not to follow symlinks.
  return status(Path);
}

Note that there's also a hole in FileSystem::dir_begin which also always follows symlinks; perhaps the parallel features could be implemented (or not) at the same time.

That said, I agree it'd be nice to have a use case for this before expanding the API.