Page MenuHomePhabricator

[VFS] Add support for "no_push" to VFS recursive iterators.
ClosedPublic

Authored by JDevlieghere on Oct 19 2018, 6:30 PM.

Details

Summary

The "regular" file system has a useful feature that makes it possible to stop recursing when using the recursive directory iterators. This functionality was missing for the VFS recursive iterator and this patch adds that.

Diff Detail

Event Timeline

JDevlieghere created this revision.Oct 19 2018, 6:30 PM
JDevlieghere edited the summary of this revision. (Show Details)Oct 19 2018, 6:32 PM
JDevlieghere retitled this revision from [vfs] Add support for "no_push to vfs recursive iterators. to [VFS] Add support for "no_push" to VFS recursive iterators..Oct 19 2018, 6:39 PM

I am gravitating towards having a separate unit test(s) for no_push functionality. Maybe I'm wrong but I expect it to be smaller and easier to understand, though there'll be some boilerplate.

Implementation looks reasonable but I'm not sure about a few edge cases:

  • call no_push before starting iteration;
  • call no_push after we finished iteration (here I mean when we are pointing to the last directory, before the iterator becomes end iterator);
  • call no_push for a directory that has no children and we wouldn't descend into anyway.

I think in these cases VFS iterator should behave as llvm::sys::fs::recursive_directory_iterator unless it is doing something strange. Not sure all these cases deserve a test, depends on the fact if they execute different paths in implementation.

llvm/unittests/Support/VirtualFileSystemTest.cpp
484–486

Is Counts[5] supposed to be // f?

JDevlieghere marked an inline comment as done.

Thanks for the review, Volodymyr! I've extended the test which I believe covers all the cases you mentioned. The implementation is almost identical to how no_push is handled in the non-VFS filesystem implementation, so the behavior should indeed be identical.

vsapsai accepted this revision.Oct 31 2018, 4:03 PM

Looks good to me.

This revision is now accepted and ready to land.Oct 31 2018, 4:03 PM
This revision was automatically updated to reflect the committed changes.