Page MenuHomePhabricator

[FileSpec] Add support for lambdas to EnumerateDirectory. NFC
AbandonedPublic

Authored by JDevlieghere on May 30 2018, 7:44 AM.

Details

Summary

Support both lambda's and function pointers as arguments to EnumerateDirectory.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.May 30 2018, 7:44 AM

Could we just get rid of the baton version?

Could we just get rid of the baton version?

It's the only way the function is used currently. How about just phasing it out when we touch the relevant code?

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

JDevlieghere added a comment.EditedMay 30 2018, 9:06 AM

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

My motivation is D47539. I could use the iterators directly but since the FileSpec one is based on them anyway (and adds some functionality that is actually useful) I figured I might as well use them for consistency. I'm not opposed to using the iterators directly, but won't that result in more code?

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

My motivation is D47539. I could use the iterators directly but since the FileSpec one is based on them anyway (and adds some functionality that is actually useful) I figured I might as well use them for consistency. I'm not opposed to using the iterators directly, but won't that result in more code?

Looking back at the last refactor of this function (https://reviews.llvm.org/D30807) I think the intention even then was to deprecate/remove it altogether.

Also, I don't think that this would increase the amount of code. Looking at your patch, it seems that it could be equivalently implemented using llvm iterators as:

std::error_code EC;
for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(), EC), End; Iter != End && !EC ; Iter.incement(EC)) {
  auto Status = Iter->status();
  if (!Status)
    break;
  if (llvm::sys::fs::is_regular_file(*Status) && llvm::sys::fs::can_execute(Status->path())
    executables.push_back(FileSpec(Status->path()));
}

which is (a tiny bit) shorter. I also find code with no callbacks more readable.

Actually, I wonder if we shouldn't just deprecate this function altogether. What was your motivation for this patch? It seems we already have llvm::fs::(recursive_)directory_iterator for this purpose. It would be great if we could use that for all new code. Have you looked at that?

My motivation is D47539. I could use the iterators directly but since the FileSpec one is based on them anyway (and adds some functionality that is actually useful) I figured I might as well use them for consistency. I'm not opposed to using the iterators directly, but won't that result in more code?

Looking back at the last refactor of this function (https://reviews.llvm.org/D30807) I think the intention even then was to deprecate/remove it altogether.

Also, I don't think that this would increase the amount of code. Looking at your patch, it seems that it could be equivalently implemented using llvm iterators as:

std::error_code EC;
for(llvm::sys::fs::recursive_directory_iterator Iter(dir.GetStringRef(), EC), End; Iter != End && !EC ; Iter.incement(EC)) {
  auto Status = Iter->status();
  if (!Status)
    break;
  if (llvm::sys::fs::is_regular_file(*Status) && llvm::sys::fs::can_execute(Status->path())
    executables.push_back(FileSpec(Status->path()));
}

which is (a tiny bit) shorter. I also find code with no callbacks more readable.

Fair enough, I'll update the patch :-)

+1 I’d like to get rid of all EnumerateXXX with callback functions and
replace with iterator based equivalents. Given that in this case the
iterator version already exists, I definitely think we should try to use it
instead

JDevlieghere abandoned this revision.May 30 2018, 9:42 AM

Alright, let's abandon this and replace the existing uses with llvm iterators.