[FileSpec] Add support for lambdas to EnumerateDirectory. NFC

[JDevlieghere]

Summary

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

[labath]

Could we just get rid of the baton version?

[JDevlieghere]

In 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?

[labath]

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]

In 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 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?

[labath]

In D47535#1116392, @JDevlieghere wrote:

In 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 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.

[JDevlieghere]

In D47535#1116430, @labath wrote:

In D47535#1116392, @JDevlieghere wrote:

In 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 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 :-)

[zturner]

+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]

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

include/lldb/Utility/FileSpec.h

public:
   static void EnumerateDirectory(llvm::StringRef dir_path,
                                  bool find_directories, bool find_files,
                                  bool find_other,
                                  EnumerateDirectoryCallbackType callback,
                                  void *callback_baton);
 
   typedef std::function<EnumerateDirectoryResult(
       llvm::sys::fs::file_type file_type, const FileSpec &spec)>
-      DirectoryCallback;
+      EnumerateDirectoryCallbackFnType;
+
+  static void EnumerateDirectory(llvm::StringRef dir_path,
+                                 bool find_directories, bool find_files,
+                                 bool find_other,
+                                 EnumerateDirectoryCallbackFnType callback);
 
 protected:
   //------------------------------------------------------------------
   // Member variables
   //------------------------------------------------------------------
   ConstString m_directory;            ///< The uniqued directory path
   ConstString m_filename;             ///< The uniqued filename path
   mutable bool m_is_resolved = false; ///< True if this path has been resolved.

source/Utility/FileSpec.cpp

size_t FileSpec::MemorySize() const {
   return m_filename.MemorySize() + m_directory.MemorySize();
 }
 
 void FileSpec::EnumerateDirectory(llvm::StringRef dir_path,
                                   bool find_directories, bool find_files,
                                   bool find_other,
                                   EnumerateDirectoryCallbackType callback,
                                   void *callback_baton) {
+  auto callbackLambda = [=](llvm::sys::fs::file_type file_type,
+                            const FileSpec &spec) -> 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);
   fs::recursive_directory_iterator End;
   for (; Iter != End && !EC; Iter.increment(EC)) {
     const auto &Item = *Iter;
     llvm::ErrorOr<fs::basic_file_status> Status = Item.status();
     if (!Status)
       break;
     if (!find_files && fs::is_regular_file(*Status))
       continue;
     if (!find_directories && fs::is_directory(*Status))
       continue;
     if (!find_other && fs::is_other(*Status))
       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) {
       // Default behavior is to recurse.  Opt out if the callback doesn't want
       // this behavior.
       Iter.no_push();
     }
   }