Wrap edit line configuration calls into helper functions

[nealsid]

Summary

Currently we call el_set directly to configure the editor in the libedit wrapper. There are some cases in which this causes extra casting, but we pass captureless lambdas as function pointers, which should work out of the box. Since el_set takes varargs, if the cast is incorrect or if the cast is not present, it causes a run time failure rather than compile error. This change makes it so a few different types of configuration is done inside a helper function to provide type safety and eliminate that casting. I didn't do all edit line configuration because I'm not sure how important it was in other cases and it might require something more general keep up with libedit's signature. I'm open to suggestions, though.

[teemperor]

LGTM, thanks!

I can to land this for you, but first let's wait a bit to see if anyone else has any comments.

[JDevlieghere]

LGTM. I wonder if we would want to wrap this in a macro to get rid of the EditLineConstString duplication while keeping the type safety.

[nealsid]

In D101250#2718107, @JDevlieghere wrote:

LGTM. I wonder if we would want to wrap this in a macro to get rid of the EditLineConstString duplication while keeping the type safety.

Thanks! I looked into removing the EditLineConstString, but I wasn't a fan of having two preprocessor macro expansions. Maybe it could be a template function wchar_t or char. I also tried some template specialization where the functions to call into the edit line library could be instantiated when used, but it didn't really appear to save much because all the method signatures for edit line parameters have to be manually maintained, and some of them still take varargs even after specifying the edit line op parameter., e.g.:

template<int N> void editLineCaller();

template<>
void editLineCaller<EL_ADDFN>(param1Type param1, param2Type param2) {

el_set(EL_ADFN, param1, param2);

. . .
}

[nealsid]

Friendly ping - let me know if I misunderstood the comments or if anything else is needed! Thanks again.

[JDevlieghere]

In D101250#2726592, @nealsid wrote:

Friendly ping - let me know if I misunderstood the comments or if anything else is needed! Thanks again.

Thanks for the explanation, I don't have a strong opinion on the matter and the current patch is an improvement so let's ship it.

lldb/include/lldb/Host/Editline.h

private:
   /// Ensures that the current EditLine instance is properly configured for
   /// single or multi-line editing.
   void ConfigureEditor(bool multiline);
 
   bool CompleteCharacter(char ch, EditLineGetCharType &out);
 
   void ApplyTerminalSizeChange();
 
+  // The following set various editline parameters.  It's not any less
+  // verbose to put the editline calls into a function, but it
+  // provides type safety, since the editline functions take varargs
+  // parameters.
+  void AddFunctionToEditLine(const EditLineCharType *command,
+                             const EditLineCharType *helptext,
+                             EditlineCommandCallbackType callbackFn);
+  void SetEditLinePromptCallback(EditlinePromptCallbackType callbackFn);
+  void SetGetCharacterFunction(EditlineGetCharCallbackType callbackFn);
+
 #if LLDB_EDITLINE_USE_WCHAR
   std::wstring_convert<std::codecvt_utf8<wchar_t>> m_utf8conv;
 #endif
   ::EditLine *m_editline = nullptr;
   EditlineHistorySP m_history_sp;
   bool m_in_history = false;
   std::vector<EditLineStringType> m_live_history_lines;
   bool m_multiline_enabled = false;

lldb/source/Host/common/Editline.cpp

int toColumn =
         editline_cursor_position - (editline_cursor_row * m_terminal_width);
     fprintf(m_output_file, ANSI_SET_COLUMN_N, toColumn);
     return CC_REFRESH;
   }
 
   return CC_REDISPLAY;
 }
 
+void Editline::AddFunctionToEditLine(const EditLineCharType *command,
+                                     const EditLineCharType *helptext,
+                                     EditlineCommandCallbackType callbackFn) {
+  el_wset(m_editline, EL_ADDFN, command, helptext, callbackFn);
+}
+
+void Editline::SetEditLinePromptCallback(
+    EditlinePromptCallbackType callbackFn) {
+  el_set(m_editline, EL_PROMPT, callbackFn);
+}
+
+void Editline::SetGetCharacterFunction(EditlineGetCharCallbackType callbackFn) {
+  el_wset(m_editline, EL_GETCFN, callbackFn);
+}
+
 void Editline::ConfigureEditor(bool multiline) {
   if (m_editline && m_multiline_enabled == multiline)
     return;
   m_multiline_enabled = multiline;
 
   if (m_editline) {
     // Disable edit mode to stop the terminal from flushing all input during
     // the call to el_end() since we expect to have multiple editline instances
if (m_history_sp && m_history_sp->IsValid()) {
     if (!m_history_sp->Load()) {
         fputs("Could not load history file\n.", m_output_file);
     }
     el_wset(m_editline, EL_HIST, history, m_history_sp->GetHistoryPtr());
   }
   el_set(m_editline, EL_CLIENTDATA, this);
   el_set(m_editline, EL_SIGNAL, 0);
   el_set(m_editline, EL_EDITOR, "emacs");
-  el_set(m_editline, EL_PROMPT,
-         (EditlinePromptCallbackType)([](EditLine *editline) {
-           return Editline::InstanceFor(editline)->Prompt();
-         }));
 
-  el_wset(m_editline, EL_GETCFN, (EditlineGetCharCallbackType)([](
-                                     EditLine *editline, EditLineGetCharType *c) {
-            return Editline::InstanceFor(editline)->GetCharacter(c);
-          }));
+  SetGetCharacterFunction([](EditLine *editline, EditLineGetCharType *c) {
+  });
+
+  SetEditLinePromptCallback([](EditLine *editline) {
+    return Editline::InstanceFor(editline)->Prompt();
+  });
 
   // Commands used for multiline support, registered whether or not they're
   // used
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-break-line"),
-          EditLineConstString("Insert a line break"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->BreakLineCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-end-or-add-line"),
-          EditLineConstString("End editing or continue when incomplete"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->EndOrAddLineCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-delete-next-char"),
-          EditLineConstString("Delete next character"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->DeleteNextCharCommand(ch);
-          }));
-  el_wset(
-      m_editline, EL_ADDFN, EditLineConstString("lldb-delete-previous-char"),
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-break-line"),
+      [](EditLine *editline, int ch) {
+      });
+
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-end-or-add-line"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-delete-next-char"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-delete-previous-char"),
       EditLineConstString("Delete previous character"),
-      (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
+      [](EditLine *editline, int ch) {
         return Editline::InstanceFor(editline)->DeletePreviousCharCommand(ch);
-      }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-previous-line"),
-          EditLineConstString("Move to previous line"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->PreviousLineCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-next-line"),
-          EditLineConstString("Move to next line"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->NextLineCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-previous-history"),
-          EditLineConstString("Move to previous history"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->PreviousHistoryCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-next-history"),
-          EditLineConstString("Move to next history"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->NextHistoryCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-buffer-start"),
-          EditLineConstString("Move to start of buffer"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->BufferStartCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-buffer-end"),
-          EditLineConstString("Move to end of buffer"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->BufferEndCommand(ch);
-          }));
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-fix-indentation"),
-          EditLineConstString("Fix line indentation"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->FixIndentationCommand(ch);
-          }));
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-previous-line"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-next-line"),
+      EditLineConstString("Move to next line"), [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-previous-history"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-next-history"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-buffer-start"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-buffer-end"),
+      [](EditLine *editline, int ch) {
+      });
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-fix-indentation"),
+      [](EditLine *editline, int ch) {
+      });
 
   // Register the complete callback under two names for compatibility with
   // older clients using custom .editrc files (largely because libedit has a
   // bad bug where if you have a bind command that tries to bind to a function
   // name that doesn't exist, it can corrupt the heap and crash your process
   // later.)
   EditlineCommandCallbackType complete_callback = [](EditLine *editline,
                                                      int ch) {
     return Editline::InstanceFor(editline)->TabCommand(ch);
   };
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-complete"),
-          EditLineConstString("Invoke completion"), complete_callback);
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb_complete"),
-          EditLineConstString("Invoke completion"), complete_callback);
+  AddFunctionToEditLine(EditLineConstString("lldb-complete"),
+                        EditLineConstString("Invoke completion"),
+                        complete_callback);
+  AddFunctionToEditLine(EditLineConstString("lldb_complete"),
+                        EditLineConstString("Invoke completion"),
+                        complete_callback);
 
   // General bindings we don't mind being overridden
   if (!multiline) {
     el_set(m_editline, EL_BIND, "^r", "em-inc-search-prev",
            NULL); // Cycle through backwards search, entering string
 
     if (m_suggestion_callback) {
-      el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-apply-complete"),
-              EditLineConstString("Adopt autocompletion"),
-              (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-                return Editline::InstanceFor(editline)->ApplyAutosuggestCommand(
-                    ch);
-              }));
+      AddFunctionToEditLine(
+          EditLineConstString("lldb-apply-complete"),
+          [](EditLine *editline, int ch) {
+            return Editline::InstanceFor(editline)->ApplyAutosuggestCommand(ch);
+          });
 
       el_set(m_editline, EL_BIND, "^f", "lldb-apply-complete",
              NULL); // Apply a part that is suggested automatically
 
-      el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-typed-character"),
-              EditLineConstString("Typed character"),
-              (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-                return Editline::InstanceFor(editline)->TypedCharacter(ch);
-              }));
+      AddFunctionToEditLine(
+          EditLineConstString("lldb-typed-character"),
+          [](EditLine *editline, int ch) {
+          });
 
       char bind_key[2] = {0, 0};
       llvm::StringRef ascii_chars =
           "abcdefghijklmnopqrstuvwxzyABCDEFGHIJKLMNOPQRSTUVWXZY1234567890!\"#$%"
           "&'()*+,./:;<=>?@[]_`{|}~ ";
       for (char c : ascii_chars) {
         bind_key[0] = c;
         el_set(m_editline, EL_BIND, bind_key, "lldb-typed-character", NULL);
void Editline::ConfigureEditor(bool multiline) {
   el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[C", "em-next-word", NULL);
   el_set(m_editline, EL_BIND, ESCAPE ESCAPE "[D", "ed-prev-word", NULL);
 
   // Allow user-specific customization prior to registering bindings we
   // absolutely require
   el_source(m_editline, nullptr);
 
   // Register an internal binding that external developers shouldn't use
-  el_wset(m_editline, EL_ADDFN, EditLineConstString("lldb-revert-line"),
-          EditLineConstString("Revert line to saved state"),
-          (EditlineCommandCallbackType)([](EditLine *editline, int ch) {
-            return Editline::InstanceFor(editline)->RevertLineCommand(ch);
-          }));
+  AddFunctionToEditLine(
+      EditLineConstString("lldb-revert-line"),
+      [](EditLine *editline, int ch) {
+      });
 
   // Register keys that perform auto-indent correction
   if (m_fix_indentation_callback && m_fix_indentation_callback_chars) {
     char bind_key[2] = {0, 0};
     const char *indent_chars = m_fix_indentation_callback_chars;
     while (*indent_chars) {
       bind_key[0] = *indent_chars;
       el_set(m_editline, EL_BIND, bind_key, "lldb-fix-indentation", NULL);