[PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC

Jonas Devlieghere via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 30 09:35:54 PDT 2018


JDevlieghere added a comment.

In https://reviews.llvm.org/D47535#1116430, @labath wrote:

> In https://reviews.llvm.org/D47535#1116392, @JDevlieghere wrote:
>
> > In https://reviews.llvm.org/D47535#1116364, @labath wrote:
> >
> > > 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 https://reviews.llvm.org/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 :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D47535





More information about the llvm-commits mailing list