[EditLine] Fix RecallHistory that makes it move in the opposite direction.

[JDevlieghere]

Summary

The naming used by editline for the history operations is counter
intuitive to how it's used in lldb for the REPL.

• The H_PREV operation returns the previous element in the history, which is newer than the current one.
• The H_NEXT operation returns the next element in the history, which is older than the current one.

This exposed itself as a bug in the REPL where the behavior of up- and
down-arrow was inverted. This wasn't immediately obvious because of how we save
the current "live" entry.

This patch fixes the bug and introduces and enum to wrap the editline
operations that match the semantics of lldb.

[JDevlieghere]

(No test because this code is only used by the REPL)

[labath]

In D70932#1766190, @JDevlieghere wrote:

... this code is only used by the REPL)

Are you sure about that? I think I was able to trigger something like this by entering the multiline expression mode (expr) , and then pressing alt+up/down. I'm not 100% sure of that's the thing you're fixing here, because i've never used this feature (though it is pretty cool), but my impression is that this is it.

[teemperor]

Yes, multiline expressions use this code, but I don't see any functional change in this patch that could be tested? It seems to only change the bool to an enum so that we can pass in Swift Newer/Older instead of cryptic true/false (and the actual fix is swapping this in swift-lldb).

Anyway, I added a test for going up/down in multiline expressions in rG4821d2a014e02b14223676c98b2ef5244eb91da8. As this seems NFC and the changed code is now tested, this can land.

[labath]

In D70932#1766846, @teemperor wrote:

Yes, multiline expressions use this code, but I don't see any functional change in this patch that could be tested? It seems to only change the bool to an enum so that we can pass in Swift Newer/Older instead of cryptic true/false (and the actual fix is swapping this in swift-lldb).

Anyway, I added a test for going up/down in multiline expressions in rG4821d2a014e02b14223676c98b2ef5244eb91da8. As this seems NFC and the changed code is now tested, this can land.

Yes, that makes sense. And thank you for adding the test!

lldb/include/lldb/Host/Editline.h

enum class CursorLocation {
   /// The location of the cursor on the current line in a multi-line edit
   /// session
   EditingCursor,
 
   /// The location immediately after the last character in a multi-line edit
   /// session
   BlockEnd
 };
+
+/// Operation for the history.
+enum class HistoryOperation {
+  Oldest,
+  Older,
+  Current,
+  Newer,
+  Newest
+};
 }
 
 using namespace line_editor;
 
 /// Instances of Editline provide an abstraction over libedit's EditLine
 /// facility.  Both
 /// single- and multi-line editing are supported.
 class Editline {
private:
 
   /// Save the line currently being edited
   void SaveEditedLine();
 
   /// Convert the current input lines into a UTF8 StringList
   StringList GetInputAsStringList(int line_count = UINT32_MAX);
 
   /// Replaces the current multi-line session with the next entry from history.
-  /// When the parameter is
-  /// true it will take the next earlier entry from history, when it is false it
-  /// takes the next most
-  /// recent.
-  unsigned char RecallHistory(bool earlier);
+  unsigned char RecallHistory(HistoryOperation op);
 
   /// Character reading implementation for EditLine that supports our multi-line
   /// editing trickery.
   int GetCharacter(EditLineGetCharType *c);
 
   /// Prompt implementation for EditLine.
   const char *Prompt();
 

lldb/source/Host/common/Editline.cpp

 bool IsOnlySpaces(const EditLineStringType &content) {
   for (wchar_t ch : content) {
     if (ch != EditLineCharType(' '))
       return false;
   }
   return true;
 }
 
+static int GetOperation(HistoryOperation op) {
+  // The naming used by editline for the history operations is counter
+  // intuitive to how it's used here.
+  //
+  //  - The H_PREV operation returns the previous element in the history, which
+  //    is newer than the current one.
+  //
+  //  - The H_NEXT operation returns the next element in the history, which is
+  //    older than the current one.
+  //
+  // The naming of the enum entries match the semantic meaning.
+  switch(op) {
+    case HistoryOperation::Oldest:
+      return H_FIRST;
+    case HistoryOperation::Older:
+      return H_NEXT;
+    case HistoryOperation::Current:
+      return H_CURR;
+    case HistoryOperation::Newer:
+      return H_PREV;
+    case HistoryOperation::Newest:
+      return H_LAST;
+  }
+}
+
+
 EditLineStringType CombineLines(const std::vector<EditLineStringType> &lines) {
   EditLineStringStreamType combined_stream;
   for (EditLineStringType line : lines) {
     combined_stream << line.c_str() << "\n";
   }
   return combined_stream.str();
 }
 

 #else
     lines.AppendString(line);
 #endif
     --line_count;
   }
   return lines;
 }
 
-unsigned char Editline::RecallHistory(bool earlier) {
+unsigned char Editline::RecallHistory(HistoryOperation op) {
+  assert(op == HistoryOperation::Older || op == HistoryOperation::Newer);
   if (!m_history_sp || !m_history_sp->IsValid())
     return CC_ERROR;
 
   HistoryW *pHistory = m_history_sp->GetHistoryPtr();
   HistEventW history_event;
   std::vector<EditLineStringType> new_input_lines;
 
   // Treat moving from the "live" entry differently
   if (!m_in_history) {
-    if (!earlier)
+    switch (op) {
+    case HistoryOperation::Newer:
       return CC_ERROR; // Can't go newer than the "live" entry
-    if (history_w(pHistory, &history_event, H_FIRST) == -1)
+    case HistoryOperation::Older: {
+      if (history_w(pHistory, &history_event,
+                    GetOperation(HistoryOperation::Newest)) == -1)
         return CC_ERROR;
-
       // Save any edits to the "live" entry in case we return by moving forward
       // in history (it would be more bash-like to save over any current entry,
-    // but libedit doesn't offer the ability to add entries anywhere except the
-    // end.)
+      // but libedit doesn't offer the ability to add entries anywhere except
+      // the end.)
       SaveEditedLine();
       m_live_history_lines = m_input_lines;
       m_in_history = true;
+    } break;
+    default:
+      llvm_unreachable("unsupported history direction");
+    }
   } else {
-    if (history_w(pHistory, &history_event, earlier ? H_PREV : H_NEXT) == -1) {
-      // Can't move earlier than the earliest entry
-      if (earlier)
+    if (history_w(pHistory, &history_event, GetOperation(op)) == -1) {
+      switch (op) {
+      case HistoryOperation::Older:
+        // Can't move earlier than the earliest entry.
         return CC_ERROR;
-
-      // ... but moving to newer than the newest yields the "live" entry
+      case HistoryOperation::Newer:
+        // Moving to newer-than-the-newest entry yields the "live" entry.
         new_input_lines = m_live_history_lines;
         m_in_history = false;
+        break;
+      default:
+        llvm_unreachable("unsupported history direction");
+      }
     }
   }
 
   // If we're pulling the lines from history, split them apart
   if (m_in_history)
     new_input_lines = SplitLines(history_event.str);
 
   // Erase the current edit session and replace it with a new one
   MoveCursor(CursorLocation::EditingCursor, CursorLocation::BlockStart);
   m_input_lines = new_input_lines;
   DisplayInput();
 
   // Prepare to edit the last line when moving to previous entry, or the first
   // line when moving to next entry
-  SetCurrentLine(m_current_line_index =
-                     earlier ? (int)m_input_lines.size() - 1 : 0);
+  switch (op) {
+  case HistoryOperation::Older:
+    m_current_line_index = (int)m_input_lines.size() - 1;
+    break;
+  case HistoryOperation::Newer:
+    m_current_line_index = 0;
+    break;
+  default:
+    llvm_unreachable("unsupported history direction");
+  }
+  SetCurrentLine(m_current_line_index);
   MoveCursor(CursorLocation::BlockEnd, CursorLocation::EditingPrompt);
   return CC_NEWLINE;
 }
 
 int Editline::GetCharacter(EditLineGetCharType *c) {
   const LineInfoW *info = el_wline(m_editline);
 
   // Paint a faint version of the desired prompt over the version libedit draws
unsigned char Editline::DeletePreviousCharCommand(int ch) {
   el_winsertstr(m_editline, priorLine.c_str());
   return CC_REDISPLAY;
 }
 
 unsigned char Editline::PreviousLineCommand(int ch) {
   SaveEditedLine();
 
   if (m_current_line_index == 0) {
-    return RecallHistory(true);
+    return RecallHistory(HistoryOperation::Older);
   }
 
   // Start from a known location
   MoveCursor(CursorLocation::EditingCursor, CursorLocation::EditingPrompt);
 
   // Treat moving up from a blank last line as a deletion of that line
   if (m_current_line_index == m_input_lines.size() - 1 && IsOnlySpaces()) {
     m_input_lines.erase(m_input_lines.begin() + m_current_line_index);

 unsigned char Editline::NextLineCommand(int ch) {
   SaveEditedLine();
 
   // Handle attempts to move down from the last line
   if (m_current_line_index == m_input_lines.size() - 1) {
     // Don't add an extra line if the existing last line is blank, move through
     // history instead
     if (IsOnlySpaces()) {
-      return RecallHistory(false);
+      return RecallHistory(HistoryOperation::Newer);
     }
 
     // Determine indentation for the new line
     int indentation = 0;
     if (m_fix_indentation_callback) {
       StringList lines = GetInputAsStringList();
       lines.AppendString("");
       indentation = m_fix_indentation_callback(
for (int line_count = 0; line_count < m_current_line_rows - cursor_row;
     fprintf(m_output_file, "\n");
   }
   return CC_NEWLINE;
 }
 
 unsigned char Editline::PreviousHistoryCommand(int ch) {
   SaveEditedLine();
 
-  return RecallHistory(true);
+  return RecallHistory(HistoryOperation::Older);
 }
 
 unsigned char Editline::NextHistoryCommand(int ch) {
   SaveEditedLine();
 
-  return RecallHistory(false);
+  return RecallHistory(HistoryOperation::Newer);
 }
 
 unsigned char Editline::FixIndentationCommand(int ch) {
   if (!m_fix_indentation_callback)
     return CC_NORM;
 
   // Insert the character typed before proceeding
   EditLineCharType inserted[] = {(EditLineCharType)ch, 0};