Migrate to llvm::unique_function instead of static member functions for callbacks

[nealsid]

Summary

A few cleanups suggested in another patch review's comments:

1. Use llvm:unique_function for storing & invoking callbacks from Editline to IOHandler
2. Change return type of one of the callback setters from bool to void, since it's return value was never used
3. Moved the callback setters inline & made them nonstatic, since that's more consistent with other setter definitions
4. Removed the baton parameter since we no longer need it anymore

[nealsid]

Fix unit test build

[zturner]

include/lldb/Host/Editline.h

94

Try llvm::unique_function instead of std::function (in other locations as well). I should have mentioned this earlier but forgot. It's basically the same except it doesn't require the callable to be copy constructible, which enables some optimizations. Not that this is a hot-path, but if we can use something strictly better, might as well.

179

= std::move(callback)

source/Core/IOHandler.cpp

314–315

I think we should avoid std::bind. Instead, just write

m_editline_ap->SetIsInputCompleteCallback([this](EditLine* a, StringList& b) { return IsInputCompleteCallback(a, b); });

similary for other places. I'd argue that std::bind actually makes the code *worse* since it's so hard to construct these bind statements and figure out what they do.

[Eugene.Zelenko]

include/lldb/Host/Editline.h

92

Please use using. Same in other places.

source/Core/IOHandler.cpp

315

See also Clang-tidy modernize-avoid-bind.

[nealsid]

Thanks for the comments! Will address and resend for review.

[nealsid]

include/lldb/Host/Editline.h

94

Ok, cool - so if I understand the optimizations we're trying to make, we want to make sure to use move semantics if were can. Doesn't std::function use move semantics when constructing itself from rvalue references to lambdas? Even if it didn't, would we benefit from llvm::unique_function since our cases are pretty simple?

[zturner]

std::function might be able to construct itself efficiently from a lambda,
but once it’s in the std::function all that info is lost and moving it
around more will always copy (unless you explicitly move). We only really
assign it once, so this isn’t an issue, but unique_function is nice from a
purist point of view in that it won’t ever allow std::function’s slow path
to be taken, even accidentally.

In practice this is a non issue because we’re talking about a couple of
cycles in code that isn’t even in a hot path, but it’s still nice for the
same reason you might use a singly linked list instead of a doubly linked
for a queue. It gives you as much as you need and nothing more.

So the benefit isn’t really one of performance. It’s more just a mindset
of, rather than saying “I’ll use the fast one if performance is a concern”,
saying instead “I’ll only use the slow one if it’s necessary for my use
case”

[nealsid]

Addressed code review feedback to move to llvm::unique_function instead of function pointers for callbacks and type aliases instead of typedefs.

[nealsid]

source/Core/IOHandler.cpp

315

I also ran clang-tidy and there's a few unrelated suggested cleanups to make, but I don't know what options to use to be consistent with the rest of the project.

[zturner]

Sorry for the delay, was on vacation last week.

[nealsid]

Fix 80 char line length violations

[zturner]

In D50299#1220983, @nealsid wrote:

Fix 80 char line length violations

Just curious, how did you fix them? Was it by running clang-format, or manually?

[nealsid]

Ran clang-format

[nealsid]

I'm updating this very old patch that I forgot about in 2018. This one removes the use of batons in the edit line callbacks, replaces typedefs with using declarations, and uses unique_function instead of callbacks.
Thanks,
Neal

[JDevlieghere]

Thanks Neal. Can you run clang-format again and upload the diff with context (git diff -U9999). Overall this looks pretty good.

[nealsid]

In D50299#2589289, @JDevlieghere wrote:

Thanks Neal. Can you run clang-format again and upload the diff with context (git diff -U9999). Overall this looks pretty good.

Thanks for the review. I have run clang-format and am currently running through a bunch of testing with the completion/suggestion features just to make sure. Hope to get it updated in the next couple of days.

[nealsid]

Updated diff with run of clang-format and added a check for function pointer validity. Thanks.

[JDevlieghere]

LGTM!

lldb/include/lldb/Host/Editline.h

60 ↗

(On Diff #326954)

[nealsid]

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

[JDevlieghere]

In D50299#2596008, @nealsid wrote:

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Looks good. Let me know if you don't have commit access and need me to land this for you.

[nealsid]

In D50299#2596141, @JDevlieghere wrote:

In D50299#2596008, @nealsid wrote:

Removed comment on #include line and organized the #includes to match coding guidelines better (I wasn't sure about whether the project uses blank lines in between #includes from different subproject like lldb/clang/llvm so I didn't make the change more broadly)

Looks good. Let me know if you don't have commit access and need me to land this for you.

I do not have commit access, so if you could land it, that would be great. Appreciate the review.

include/lldb/Core/IOHandler.h

 
 private:
 #ifndef LLDB_DISABLE_LIBEDIT
-  static bool IsInputCompleteCallback(Editline *editline, StringList &lines,
-                                      void *baton);
+  bool IsInputCompleteCallback(Editline *editline, StringList &lines);
 
-  static int FixIndentationCallback(Editline *editline, const StringList &lines,
-                                    int cursor_position, void *baton);
+  int FixIndentationCallback(Editline *editline, const StringList &lines,
+                             int cursor_position);
 
-  static int AutoCompleteCallback(const char *current_line, const char *cursor,
-                                  const char *last_char,
-                                  int skip_first_n_matches, int max_matches,
-                                  StringList &matches, void *baton);
+  int AutoCompleteCallback(const char *current_line, const char *cursor,
+                           const char *last_char, int skip_first_n_matches,
+                           int max_matches, StringList &matches);
 #endif
 
 protected:

include/lldb/Host/Editline.h

 
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/lldb-private.h"
+#include "llvm/ADT/FunctionExtras.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/editlinewin.h"

 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Predicate.h"
 #include "lldb/Utility/FileSpec.h"
+#include "lldb/Utility/StringList.h"   // for StringList
 
 namespace lldb_private {
 namespace line_editor {

 using EditLineGetCharType = char;
 #endif
 
-typedef int (*EditlineGetCharCallbackType)(::EditLine *editline,
+using EditlineGetCharCallbackType = int (*)(::EditLine *editline,
                                            EditLineGetCharType *c);
-typedef unsigned char (*EditlineCommandCallbackType)(::EditLine *editline,
+using EditlineCommandCallbackType = unsigned char (*)(::EditLine *editline,
                                                      int ch);
-typedef const char *(*EditlinePromptCallbackType)(::EditLine *editline);
+using EditlinePromptCallbackType = const char *(*)(::EditLine *editline);
 
 class EditlineHistory;
 
-typedef std::shared_ptr<EditlineHistory> EditlineHistorySP;
+using EditlineHistorySP = std::shared_ptr<EditlineHistory>;
 
-typedef bool (*IsInputCompleteCallbackType)(Editline *editline,
-                                            StringList &lines, void *baton);
-
-typedef int (*FixIndentationCallbackType)(Editline *editline,
-                                          const StringList &lines,
-                                          int cursor_position, void *baton);
-
-typedef int (*CompleteCallbackType)(const char *current_line,
-                                    const char *cursor, const char *last_char,
-                                    int skip_first_n_matches, int max_matches,
-                                    StringList &matches, void *baton);
+using IsInputCompleteCallbackType = llvm::unique_function<bool(Editline*, StringList&)>;
+using FixIndentationCallbackType = llvm::unique_function<int(Editline*, StringList&, int)>;
+using CompleteCallbackType = llvm::unique_function<int(const char*, const char*, const char*,
+                                                       int, int, StringList&)>;
 
 /// Status used to decide when and how to start editing another line in
 /// multi-line sessions

   bool Cancel();
 
   /// Register a callback for the tab key
-  void SetAutoCompleteCallback(CompleteCallbackType callback, void *baton);
+  void SetAutoCompleteCallback(CompleteCallbackType callback) {
+    m_completion_callback = std::move(callback);
+  }
 
   /// Register a callback for testing whether multi-line input is complete
-  void SetIsInputCompleteCallback(IsInputCompleteCallbackType callback,
-                                  void *baton);
+  void SetIsInputCompleteCallback(IsInputCompleteCallbackType callback) {
+    m_is_input_complete_callback = std::move(callback);
+  }
 
   /// Register a callback for determining the appropriate indentation for a line
   /// when creating a newline.  An optional set of insertable characters can
-  /// also
-  /// trigger the callback.
-  bool SetFixIndentationCallback(FixIndentationCallbackType callback,
-                                 void *baton, const char *indent_chars);
+  /// also trigger the callback.
+  void SetFixIndentationCallback(FixIndentationCallbackType callback,
+                                 const char *indent_chars) {
+    m_fix_indentation_callback = std::move(callback);
+    m_fix_indentation_callback_chars = indent_chars;
+  }
 
   /// Prompts for and reads a single line of user input.
   bool GetLine(std::string &line, bool &interrupted);

   FILE *m_output_file;
   FILE *m_error_file;
   ConnectionFileDescriptor m_input_connection;
-  IsInputCompleteCallbackType m_is_input_complete_callback = nullptr;
-  void *m_is_input_complete_callback_baton = nullptr;
-  FixIndentationCallbackType m_fix_indentation_callback = nullptr;
-  void *m_fix_indentation_callback_baton = nullptr;
+  IsInputCompleteCallbackType m_is_input_complete_callback;
+  FixIndentationCallbackType m_fix_indentation_callback;
   const char *m_fix_indentation_callback_chars = nullptr;
-  CompleteCallbackType m_completion_callback = nullptr;
-  void *m_completion_callback_baton = nullptr;
+  CompleteCallbackType m_completion_callback;
 
   std::mutex m_output_mutex;
 };

source/Core/IOHandler.cpp

 
 using namespace lldb;
 using namespace lldb_private;
+using namespace std::placeholders;
 
 IOHandler::IOHandler(Debugger &debugger, IOHandler::Type type)
     : IOHandler(debugger, type,

     m_editline_ap.reset(new Editline(editline_name, GetInputFILE(),
                                      GetOutputFILE(), GetErrorFILE(),
                                      m_color_prompts));
-    m_editline_ap->SetIsInputCompleteCallback(IsInputCompleteCallback, this);
-    m_editline_ap->SetAutoCompleteCallback(AutoCompleteCallback, this);
+    m_editline_ap->SetIsInputCompleteCallback(
+        [this] (Editline *editline, StringList& lines) {
+          return this->IsInputCompleteCallback(editline, lines);
+        });
+
+    m_editline_ap->SetAutoCompleteCallback(
+        [this] (const char *current_line, const char *cursor,
+                const char *last_char, int skip_first_n_matches,
+                int max_matches, StringList &matches) {
+          return this->AutoCompleteCallback(current_line, cursor, last_char,
+                                            skip_first_n_matches, max_matches,
+                                            matches);
+        });
+
     // See if the delegate supports fixing indentation
     const char *indent_chars = delegate.IOHandlerGetFixIndentationCharacters();
     if (indent_chars) {
       // The delegate does support indentation, hook it up so when any
       // indentation character is typed, the delegate gets a chance to fix it
-      m_editline_ap->SetFixIndentationCallback(FixIndentationCallback, this,
-                                               indent_chars);
+      m_editline_ap->SetFixIndentationCallback(
+          [this] (Editline *editline, const StringList &lines,
+                  int cursor_position) {
+            return this->FixIndentationCallback(editline, lines, cursor_position);
+          }, indent_chars);
     }
   }
 #endif

 
 #ifndef LLDB_DISABLE_LIBEDIT
 bool IOHandlerEditline::IsInputCompleteCallback(Editline *editline,
-                                                StringList &lines,
-                                                void *baton) {
-  IOHandlerEditline *editline_reader = (IOHandlerEditline *)baton;
-  return editline_reader->m_delegate.IOHandlerIsInputComplete(*editline_reader,
-                                                              lines);
+                                                StringList &lines) {
+  return m_delegate.IOHandlerIsInputComplete(*this, lines);
 }
 
 int IOHandlerEditline::FixIndentationCallback(Editline *editline,
                                               const StringList &lines,
-                                              int cursor_position,
-                                              void *baton) {
-  IOHandlerEditline *editline_reader = (IOHandlerEditline *)baton;
-  return editline_reader->m_delegate.IOHandlerFixIndentation(
-      *editline_reader, lines, cursor_position);
+                                              int cursor_position) {
+  return m_delegate.IOHandlerFixIndentation(
+      *this, lines, cursor_position);
 }
 
 int IOHandlerEditline::AutoCompleteCallback(const char *current_line,

                                             const char *last_char,
                                             int skip_first_n_matches,
                                             int max_matches,
-                                            StringList &matches, void *baton) {
-  IOHandlerEditline *editline_reader = (IOHandlerEditline *)baton;
-  if (editline_reader)
-    return editline_reader->m_delegate.IOHandlerComplete(
-        *editline_reader, current_line, cursor, last_char, skip_first_n_matches,
-        max_matches, matches);
-  return 0;
+                                            StringList &matches) {
+  return m_delegate.IOHandlerComplete(
+      *this, current_line, cursor, last_char, skip_first_n_matches,
+      max_matches, matches);
 }
 #endif
 

source/Host/common/Editline.cpp

       lines.AppendString(new_line_fragment);
 #endif
 
-      int indent_correction = m_fix_indentation_callback(
-          this, lines, 0, m_fix_indentation_callback_baton);
+      int indent_correction = m_fix_indentation_callback(this, lines, 0);
       new_line_fragment = FixIndentation(new_line_fragment, indent_correction);
       m_revert_cursor_index = GetIndentation(new_line_fragment);
     }

       info->cursor == info->lastchar) {
     if (m_is_input_complete_callback) {
       auto lines = GetInputAsStringList();
-      if (!m_is_input_complete_callback(this, lines,
-                                        m_is_input_complete_callback_baton)) {
+      if (!m_is_input_complete_callback(this, lines)) {
         return BreakLineCommand(ch);
       }
 

     if (m_fix_indentation_callback) {
       StringList lines = GetInputAsStringList();
       lines.AppendString("");
-      indentation = m_fix_indentation_callback(
-          this, lines, 0, m_fix_indentation_callback_baton);
+      indentation = m_fix_indentation_callback(this, lines, 0);
     }
     m_input_lines.insert(
         m_input_lines.end(),

   // Save the edits and determine the correct indentation level
   SaveEditedLine();
   StringList lines = GetInputAsStringList(m_current_line_index + 1);
-  int indent_correction = m_fix_indentation_callback(
-      this, lines, cursor_position, m_fix_indentation_callback_baton);
+  int indent_correction = m_fix_indentation_callback(this, lines,
+                                                     cursor_position);
 
   // If it is already correct no special work is needed
   if (indent_correction == 0)

 }
 
 unsigned char Editline::TabCommand(int ch) {
-  if (m_completion_callback == nullptr)
+  if (!m_completion_callback)
     return CC_ERROR;
 
   const LineInfo *line_info = el_line(m_editline);

       line_info->buffer, line_info->cursor, line_info->lastchar,
       0,  // Don't skip any matches (start at match zero)
       -1, // Get all the matches
-      completions, m_completion_callback_baton);
+      completions);
 
   if (num_completions == 0)
     return CC_ERROR;

   return result;
 }
 
-void Editline::SetAutoCompleteCallback(CompleteCallbackType callback,
-                                       void *baton) {
-  m_completion_callback = callback;
-  m_completion_callback_baton = baton;
-}
-
-void Editline::SetIsInputCompleteCallback(IsInputCompleteCallbackType callback,
-                                          void *baton) {
-  m_is_input_complete_callback = callback;
-  m_is_input_complete_callback_baton = baton;
-}
-
-bool Editline::SetFixIndentationCallback(FixIndentationCallbackType callback,
-                                         void *baton,
-                                         const char *indent_chars) {
-  m_fix_indentation_callback = callback;
-  m_fix_indentation_callback_baton = baton;
-  m_fix_indentation_callback_chars = indent_chars;
-  return false;
-}
-
 bool Editline::GetLine(std::string &line, bool &interrupted) {
   ConfigureEditor(false);
   m_input_lines = std::vector<EditLineStringType>();

unittests/Editline/EditlineTest.cpp

   void ConsumeAllOutput();
 
 private:
-  static bool IsInputComplete(lldb_private::Editline *editline,
-                              lldb_private::StringList &lines, void *baton);
+  bool IsInputComplete(lldb_private::Editline *editline,
+                       lldb_private::StringList &lines);
 
   std::unique_ptr<lldb_private::Editline> _editline_sp;
 

   _editline_sp->SetPrompt("> ");
 
   // Hookup our input complete callback.
-  _editline_sp->SetIsInputCompleteCallback(IsInputComplete, this);
+  _editline_sp->SetIsInputCompleteCallback([this] (Editline *editline, StringList &lines) {
+                                             return this->IsInputComplete(editline, lines);
+                                           });
 }
 
 void EditlineAdapter::CloseInput() {

 }
 
 bool EditlineAdapter::IsInputComplete(lldb_private::Editline *editline,
-                                      lldb_private::StringList &lines,
-                                      void *baton) {
+                                      lldb_private::StringList &lines) {
   // We'll call ourselves complete if we've received a balanced set of braces.
   int start_block_count = 0;
   int brace_balance = 0;