[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
zturner added a subscriber: JDevlieghere. zturner added a comment. +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 Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
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
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
+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
On Wed, May 30, 2018 at 9:30 AM Pavel Labath via Phabricator <
revi...@reviews.llvm.org> wrote:
> labath added a reviewer: zturner.
> labath added a comment.
>
> 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.
>
>
> Repository:
> rL LLVM
>
> https://reviews.llvm.org/D47535
>
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a reviewer: zturner.
labath added a comment.
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.
Repository:
rL LLVM
https://reviews.llvm.org/D47535
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere added a comment. 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'll result in more code? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a comment. 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? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere added a comment. In https://reviews.llvm.org/D47535#1116274, @labath wrote: > 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? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
labath added a comment. Could we just get rid of the baton version? Repository: rL LLVM https://reviews.llvm.org/D47535 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D47535: [FileSpec] Add support for lambdas to EnumerateDirectory. NFC
JDevlieghere created this revision.
JDevlieghere added reviewers: clayborg, labath.
Herald added a subscriber: llvm-commits.
Support both lambda's and function pointers as arguments to EnumerateDirectory.
Repository:
rL LLVM
https://reviews.llvm.org/D47535
Files:
include/lldb/Utility/FileSpec.h
source/Utility/FileSpec.cpp
Index: source/Utility/FileSpec.cpp
===
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -640,6 +640,18 @@
bool find_other,
EnumerateDirectoryCallbackType callback,
void *callback_baton) {
+ auto callbackLambda = [=](llvm::sys::fs::file_type file_type,
+const FileSpec ) -> EnumerateDirectoryResult {
+return callback(callback_baton, file_type, spec);
+ };
+ return EnumerateDirectory(dir_path, find_directories, find_files, find_other,
+callbackLambda);
+}
+
+void FileSpec::EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback) {
namespace fs = llvm::sys::fs;
std::error_code EC;
fs::recursive_directory_iterator Iter(dir_path, EC);
@@ -657,7 +669,7 @@
continue;
FileSpec Spec(Item.path(), false);
-auto Result = callback(callback_baton, Status->type(), Spec);
+auto Result = callback(Status->type(), Spec);
if (Result == eEnumerateDirectoryResultQuit)
return;
if (Result == eEnumerateDirectoryResultNext) {
Index: include/lldb/Utility/FileSpec.h
===
--- include/lldb/Utility/FileSpec.h
+++ include/lldb/Utility/FileSpec.h
@@ -562,7 +562,12 @@
typedef std::function
- DirectoryCallback;
+ EnumerateDirectoryCallbackFnType;
+
+ static void EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback);
protected:
//--
Index: source/Utility/FileSpec.cpp
===
--- source/Utility/FileSpec.cpp
+++ source/Utility/FileSpec.cpp
@@ -640,6 +640,18 @@
bool find_other,
EnumerateDirectoryCallbackType callback,
void *callback_baton) {
+ auto callbackLambda = [=](llvm::sys::fs::file_type file_type,
+const FileSpec ) -> EnumerateDirectoryResult {
+return callback(callback_baton, file_type, spec);
+ };
+ return EnumerateDirectory(dir_path, find_directories, find_files, find_other,
+callbackLambda);
+}
+
+void FileSpec::EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback) {
namespace fs = llvm::sys::fs;
std::error_code EC;
fs::recursive_directory_iterator Iter(dir_path, EC);
@@ -657,7 +669,7 @@
continue;
FileSpec Spec(Item.path(), false);
-auto Result = callback(callback_baton, Status->type(), Spec);
+auto Result = callback(Status->type(), Spec);
if (Result == eEnumerateDirectoryResultQuit)
return;
if (Result == eEnumerateDirectoryResultNext) {
Index: include/lldb/Utility/FileSpec.h
===
--- include/lldb/Utility/FileSpec.h
+++ include/lldb/Utility/FileSpec.h
@@ -562,7 +562,12 @@
typedef std::function
- DirectoryCallback;
+ EnumerateDirectoryCallbackFnType;
+
+ static void EnumerateDirectory(llvm::StringRef dir_path,
+ bool find_directories, bool find_files,
+ bool find_other,
+ EnumerateDirectoryCallbackFnType callback);
protected:
//--
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
