Skip to content

Commit 0e9b0b6

Browse files
committed
[EditLine] Fix RecallHistory to make it go in the right direction.
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. Differential revision: https://reviews.llvm.org/D70932
1 parent 980133a commit 0e9b0b6

File tree

2 files changed

+82
-30
lines changed

2 files changed

+82
-30
lines changed

lldb/include/lldb/Host/Editline.h

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,15 @@ enum class CursorLocation {
133133
/// session
134134
BlockEnd
135135
};
136+
137+
/// Operation for the history.
138+
enum class HistoryOperation {
139+
Oldest,
140+
Older,
141+
Current,
142+
Newer,
143+
Newest
144+
};
136145
}
137146

138147
using namespace line_editor;
@@ -258,11 +267,7 @@ class Editline {
258267
StringList GetInputAsStringList(int line_count = UINT32_MAX);
259268

260269
/// Replaces the current multi-line session with the next entry from history.
261-
/// When the parameter is
262-
/// true it will take the next earlier entry from history, when it is false it
263-
/// takes the next most
264-
/// recent.
265-
unsigned char RecallHistory(bool earlier);
270+
unsigned char RecallHistory(HistoryOperation op);
266271

267272
/// Character reading implementation for EditLine that supports our multi-line
268273
/// editing trickery.

lldb/source/Host/common/Editline.cpp

Lines changed: 72 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,32 @@ bool IsOnlySpaces(const EditLineStringType &content) {
9797
return true;
9898
}
9999

100+
static int GetOperation(HistoryOperation op) {
101+
// The naming used by editline for the history operations is counter
102+
// intuitive to how it's used here.
103+
//
104+
// - The H_PREV operation returns the previous element in the history, which
105+
// is newer than the current one.
106+
//
107+
// - The H_NEXT operation returns the next element in the history, which is
108+
// older than the current one.
109+
//
110+
// The naming of the enum entries match the semantic meaning.
111+
switch(op) {
112+
case HistoryOperation::Oldest:
113+
return H_FIRST;
114+
case HistoryOperation::Older:
115+
return H_NEXT;
116+
case HistoryOperation::Current:
117+
return H_CURR;
118+
case HistoryOperation::Newer:
119+
return H_PREV;
120+
case HistoryOperation::Newest:
121+
return H_LAST;
122+
}
123+
}
124+
125+
100126
EditLineStringType CombineLines(const std::vector<EditLineStringType> &lines) {
101127
EditLineStringStreamType combined_stream;
102128
for (EditLineStringType line : lines) {
@@ -423,7 +449,8 @@ StringList Editline::GetInputAsStringList(int line_count) {
423449
return lines;
424450
}
425451

426-
unsigned char Editline::RecallHistory(bool earlier) {
452+
unsigned char Editline::RecallHistory(HistoryOperation op) {
453+
assert(op == HistoryOperation::Older || op == HistoryOperation::Newer);
427454
if (!m_history_sp || !m_history_sp->IsValid())
428455
return CC_ERROR;
429456

@@ -433,27 +460,38 @@ unsigned char Editline::RecallHistory(bool earlier) {
433460

434461
// Treat moving from the "live" entry differently
435462
if (!m_in_history) {
436-
if (!earlier)
463+
switch (op) {
464+
case HistoryOperation::Newer:
437465
return CC_ERROR; // Can't go newer than the "live" entry
438-
if (history_w(pHistory, &history_event, H_FIRST) == -1)
439-
return CC_ERROR;
440-
441-
// Save any edits to the "live" entry in case we return by moving forward
442-
// in history (it would be more bash-like to save over any current entry,
443-
// but libedit doesn't offer the ability to add entries anywhere except the
444-
// end.)
445-
SaveEditedLine();
446-
m_live_history_lines = m_input_lines;
447-
m_in_history = true;
466+
case HistoryOperation::Older: {
467+
if (history_w(pHistory, &history_event,
468+
GetOperation(HistoryOperation::Newest)) == -1)
469+
return CC_ERROR;
470+
// Save any edits to the "live" entry in case we return by moving forward
471+
// in history (it would be more bash-like to save over any current entry,
472+
// but libedit doesn't offer the ability to add entries anywhere except
473+
// the end.)
474+
SaveEditedLine();
475+
m_live_history_lines = m_input_lines;
476+
m_in_history = true;
477+
} break;
478+
default:
479+
llvm_unreachable("unsupported history direction");
480+
}
448481
} else {
449-
if (history_w(pHistory, &history_event, earlier ? H_PREV : H_NEXT) == -1) {
450-
// Can't move earlier than the earliest entry
451-
if (earlier)
482+
if (history_w(pHistory, &history_event, GetOperation(op)) == -1) {
483+
switch (op) {
484+
case HistoryOperation::Older:
485+
// Can't move earlier than the earliest entry.
452486
return CC_ERROR;
453-
454-
// ... but moving to newer than the newest yields the "live" entry
455-
new_input_lines = m_live_history_lines;
456-
m_in_history = false;
487+
case HistoryOperation::Newer:
488+
// Moving to newer-than-the-newest entry yields the "live" entry.
489+
new_input_lines = m_live_history_lines;
490+
m_in_history = false;
491+
break;
492+
default:
493+
llvm_unreachable("unsupported history direction");
494+
}
457495
}
458496
}
459497

@@ -468,8 +506,17 @@ unsigned char Editline::RecallHistory(bool earlier) {
468506

469507
// Prepare to edit the last line when moving to previous entry, or the first
470508
// line when moving to next entry
471-
SetCurrentLine(m_current_line_index =
472-
earlier ? (int)m_input_lines.size() - 1 : 0);
509+
switch (op) {
510+
case HistoryOperation::Older:
511+
m_current_line_index = (int)m_input_lines.size() - 1;
512+
break;
513+
case HistoryOperation::Newer:
514+
m_current_line_index = 0;
515+
break;
516+
default:
517+
llvm_unreachable("unsupported history direction");
518+
}
519+
SetCurrentLine(m_current_line_index);
473520
MoveCursor(CursorLocation::BlockEnd, CursorLocation::EditingPrompt);
474521
return CC_NEWLINE;
475522
}
@@ -721,7 +768,7 @@ unsigned char Editline::PreviousLineCommand(int ch) {
721768
SaveEditedLine();
722769

723770
if (m_current_line_index == 0) {
724-
return RecallHistory(true);
771+
return RecallHistory(HistoryOperation::Older);
725772
}
726773

727774
// Start from a known location
@@ -747,7 +794,7 @@ unsigned char Editline::NextLineCommand(int ch) {
747794
// Don't add an extra line if the existing last line is blank, move through
748795
// history instead
749796
if (IsOnlySpaces()) {
750-
return RecallHistory(false);
797+
return RecallHistory(HistoryOperation::Newer);
751798
}
752799

753800
// Determine indentation for the new line
@@ -779,13 +826,13 @@ unsigned char Editline::NextLineCommand(int ch) {
779826
unsigned char Editline::PreviousHistoryCommand(int ch) {
780827
SaveEditedLine();
781828

782-
return RecallHistory(true);
829+
return RecallHistory(HistoryOperation::Older);
783830
}
784831

785832
unsigned char Editline::NextHistoryCommand(int ch) {
786833
SaveEditedLine();
787834

788-
return RecallHistory(false);
835+
return RecallHistory(HistoryOperation::Newer);
789836
}
790837

791838
unsigned char Editline::FixIndentationCommand(int ch) {

0 commit comments

Comments
 (0)