On Thursday 30 June 2011 08:35, kyak wrote:
> Implemented readline's mimic for reverse history search (Ctrl+r)
>
> Signed-off-by: kyak <[email protected]>
> ---
> libbb/Config.src | 7 ++
> libbb/lineedit.c | 193
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 200 insertions(+), 0 deletions(-)
>
> +#if ENABLE_FEATURE_REVERSE_SEARCH
> +/* mimic readline's Ctrl+r behaviour for reverse history search */
Please shortly describe the implemented behavior in this comment.
> +static smallint reverse_i_search(void)
> +{
> + /* prepare the prompt */
> + const char prmt[] = "(reverse-i-search)`";
It should be static.
> + int prmt_len = strlen(prmt);
Use sizeof(prmt)-1, it has no runtime cost.
> + char *prmt_mem_ptr = xzalloc(1);
> + /* save the real prompt */
> + char *prmt_mem_ptr_save = xzalloc(1);
> + /* user input is collected here */
> + char *match_buf;
> + /* matched lines from history are here */
> + char *cmdline_buf;
> + char read_key_buffer[KEYCODE_BUFFER_SIZE];
> + int ic;
> + char buf[MB_CUR_MAX + 1];
> + wchar_t wc[MAX_LINELEN];
> + ssize_t len, len_cmd;
> + mbstate_t mbst = { 0 };
> + smallint break_out = 0;
> + smallint break_out_search = 0;
> + smallint has_match = 0;
> + int i;
> + int cur = state->cur_history, cur_match = state->cur_history;
> + int cmdedit_y_add = 0, cmdedit_y_add_prev = 0, cmdedit_y_add_cmp = 0;
In complex cases like this, I prefer to have assignments not intermixed
with declarations.
> + read_key_buffer[0] = 0;
> + prmt_mem_ptr = strcat(xrealloc(prmt_mem_ptr, prmt_len+1), prmt);
Basically you do this:
prmt_len = strlen(prmt);
prmt_mem_ptr = xzalloc(1);
prmt_mem_ptr = strcat(xrealloc(prmt_mem_ptr, prmt_len+1), prmt);
This looks very suboptimal, should be: prmt_mem_ptr = xstrdup(prmt);
> + /* save prompt */
> + prmt_mem_ptr_save = strcat(xrealloc(prmt_mem_ptr_save,
> cmdedit_prmt_len+1), cmdedit_prompt);
> + /* overwrite the prompt */
> + cmdedit_prompt = prmt_mem_ptr;
> + cmdedit_prmt_len = prmt_len;
> + /* save what's already typed */
> + match_buf = xmalloc(MAX_LINELEN * sizeof(int16_t));
> + cmdline_buf = xmalloc(MAX_LINELEN * sizeof(int16_t));
> + save_string(match_buf, MAX_LINELEN);
Why do you think MAX_LINELEN * sizeof(int16_t) will be enough?
> + command_len = load_string("", MAX_LINELEN);
> + redraw(cmdedit_y, 0);
> + fputs(match_buf, stdout);
> + fputs("': ", stdout);
> + fputs(match_buf, stdout);
> + cmdedit_y_add_cmp = (prmt_len+1 + 2 * strlen(match_buf)) /
> (cmdedit_termw);
> + cmdedit_y_add_prev = cmdedit_y_add_cmp;
> + /* switch to char-consumption mode... */
> + while (1) {
> + fflush_all();
> + ic = lineedit_read_key(read_key_buffer, -1);
> + switch (ic) {
> + case KEYCODE_RIGHT: /* left-right keys act differently
> from Enter */
> + case KEYCODE_LEFT:
> + if (has_match == 1) {
> + command_len = load_string(cmdline_buf,
> MAX_LINELEN);
> + } else {
> + command_len = load_string(match_buf,
> MAX_LINELEN);
> + }
> + cmdedit_prompt = prmt_mem_ptr_save;
> + cmdedit_prmt_len = strlen(cmdedit_prompt);
> + redraw(cmdedit_y + cmdedit_y_add, 0);
> + break_out = 0;
> + break;
> + case CTRL('R'): /* searching for the next match */
> + break_out = 2;
> + break;
> + case CTRL('C'):
> + case KEYCODE_UP:
> + case KEYCODE_DOWN:
> + case KEYCODE_HOME:
> + case KEYCODE_END:
> + case KEYCODE_DELETE:
> + case KEYCODE_CTRL_RIGHT:
> + case KEYCODE_CTRL_LEFT:
> + case '\x1b': /* ESC */
> + command_len = load_string("", MAX_LINELEN);
> + cmdedit_prompt = prmt_mem_ptr_save;
> + redraw(cmdedit_y + cmdedit_y_add, 0);
> + break_out = 0;
> + break;
> + case '\b': /* ^H */
> + case '\x7f': /* DEL */
> + /* convert to wide char string,
> + * delete char, then convert back */
> + len = mbstowcs(wc, match_buf, sizeof(wc));
What will happen if CONFIG_UNICODE_SUPPORT is off?
> + if (len < 0)
> + len = 0;
> + wc[len-1] = '\0';
> + wcstombs(match_buf, wc, MAX_LINELEN);
> + break_out = 2;
> + has_match = 0;
> + break;
> + case '\r':
> + case '\n': /* Enter */
> + if (has_match == 1) {
> + command_len = load_string(cmdline_buf,
> MAX_LINELEN);
> + } else {
> + command_len = load_string(match_buf,
> MAX_LINELEN);
> + }
> + cmdedit_prompt = prmt_mem_ptr_save;
> + redraw(cmdedit_y + cmdedit_y_add, 0);
> + goto_new_line();
> + break_out = 1;
> + break;
> + default: /* process the char */
> + len = wcrtomb(buf, ic, &mbst);
> + if (len > 0) {
> + buf[len] = '\0';
> + }
> + strcat(match_buf, buf);
Is this overflow-safe?
> + break_out = 2;
> + break;
> + }
> + if (break_out != 2)
> + break;
> + /* if there is something in type buffer,
> + * do iterative search in history */
> + if (strlen(match_buf) > 0) {
Should be: match_buf[0] != '\0'
> + /* save current position in history */
> + cur = state->cur_history;
> + if (ic != CTRL('R')) {
> + cur_match = state->cur_history;
> + } else {
> + /* if we hit Ctrl+r (again),
> + * start searching from the last matched
> position */
> + state->cur_history = cur_match;
> + }
> + while(get_previous_history()) {
> + cmdline_buf =
> state->history[state->cur_history] ?
> + state->history[state->cur_history] :
> '\0';
cmdline_buf is a pointer. Assigning '\0' to a pointer is strange.
> + has_match = 0;
> + break_out_search = 0;
At the minimum, break_out_search should be local to
while(get_previous_history()) block.
But better if you do without it (I think you can do it using goto).
> +
> for(i=0;i+strlen(match_buf)<=strlen(cmdline_buf);i++) {
> + if (strncmp(match_buf, cmdline_buf+i,
> strlen(match_buf)) == 0) {
Can you cut down on repetitive strlen's?
> + has_match = 1;
> + break_out_search = 1;
> + /* save position of current
> match */
> + cur_match = state->cur_history;
> + break;
> + }
> + }
> + if (break_out_search == 1)
> + break;
> + }
> + }
> + /* TODO: this could be done better. There are still some (rare)
> + * cases of wrapping miscalculation */
> + len = mbstowcs(wc, match_buf, sizeof(wc));
> + if (len < 0)
> + len = strlen(match_buf);
> +
> + len_cmd = mbstowcs(wc, cmdline_buf, sizeof(wc));
> + if (len_cmd < 0)
> + len_cmd = strlen(cmdline_buf);
> +
> + cmdedit_y_add = (prmt_len+1 + len + (has_match ? len_cmd :
> len)) /
> + (cmdedit_termw);
> +
> + if (cmdedit_y_add > cmdedit_y_add_cmp) {
> + for (int j = 1; j <= (cmdedit_y_add-cmdedit_y_add_cmp);
> j++) {
> + /* scroll up */
> + printf(ESC"D");
> + }
> + if (cmdedit_y_add-cmdedit_y_add_cmp > 1)
> + redraw(cmdedit_y + cmdedit_y_add -
> cmdedit_y_add_cmp, 0);
Maybe using a temporary variable int t = cmdedit_y_add - cmdedit_y_add_cmp
will simplify code above?
--
vda
_______________________________________________
busybox mailing list
[email protected]
http://lists.busybox.net/mailman/listinfo/busybox
