This is an archive of the discontinued LLVM Phabricator instance.

Add llvm::sys::fs::remove_directories()
ClosedPublic

Authored by zturner on Mar 6 2017, 8:58 PM.

Details

Summary
We have create_directories() which can create an entire tree,
and remove(), which can remove an empty directory, but not
a remove_directories() which can remove an entire tree.

This patch adds such a function.

Diff Detail

Event Timeline

zturner created this revision.Mar 6 2017, 8:58 PM
mehdi_amini accepted this revision.Mar 6 2017, 10:05 PM

LGTM

llvm/lib/Support/Path.cpp
937

I assume removing entries in a directory does not invalidate the iterator?

This revision is now accepted and ready to land.Mar 6 2017, 10:05 PM
amccarth added inline comments.
llvm/lib/Support/Path.cpp
933

This is the usual Posix approach, so I'm concerned it won't be reliable on Windows.

When you delete files from a directory on Windows, they are marked for deletion and actually deleted later, typically milliseconds later, but it can actually be even longer. So when you try to delete the directory, it may not actually be empty yet.

The correct pattern on Windows is to rename the files out of directory (to some temp location) and then delete them. The rename, if on the same volume, is synchronous, so you know that the directory is actually empty, even if the delete hasn't happened yet. Is this what the implementation of remove does on Windows?

mehdi_amini added inline comments.Mar 7 2017, 9:30 AM
llvm/lib/Support/Path.cpp
933
zturner updated this revision to Diff 90952.Mar 7 2017, 3:37 PM

Updated to use a more robust method on Windows. Also, on non-Windows platforms unfortunately directory_iterator will follow symlinks when statting, so directory symlinks will look like ordinary directories. This is a serious problem for a recursive delete function, because we will walk into the child and delete everything in it.

To err on the side of caution, I've updated directory_iterator and recursive_directory_iterator to make following symlinks opt-out (this way all existing code is unchanged). In the deletion function specifically, I use this opt-out flag so that we do not follow symlinks, and we just delete the link itself.

Although it's not explicitly documented what the behavior of SHFileOperation is with respect to this, and despite the fact that there is a mysterious flag called SH_NORECURSEREPARSE which it claims is "unsupported", I've confirmed that with the code as written, the API does *not* recurse into reparse points (either junctions or symlinks).

amccarth accepted this revision.Mar 8 2017, 9:48 AM

LGTM.

This revision was automatically updated to reflect the committed changes.