teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.
Sorry this is taking so long, but I think beside one minor change this only
needs some testing and then it's ready.
So the way I would like to see this tested would be with a pexpect test.
Pexpect just launches a virtual terminal where LLDB runs inside and you can
send input and read output. There is a good example test here that you can copy
`lldb/test/API/commands/expression/multiline-navigation/TestMultilineNavigation.py`.
I think it's a good enough test if you just type any lldb command like `help
frame` in the test, press enter and then check that if you type like `hel` you
get the autosuggestion displayed in the terminal output. And then you press
Ctrl+F in the terminal (you have to find the escape sequence for that when
sending it to pexpect) and should see the complete autosuggestion.
Be aware that there is chance that the Pexpect terminal behaves differently
than a real terminal and you see wrong results there. In that case we need to
come up with another solution, but let's first try pexpect. Also you should
know that pexpect tests don't know if LLDB is actually still producing output
or idling, so if get timeouts while reading the output that just means we
didn't see the right output.
================
Comment at: lldb/source/Host/common/Editline.cpp:1007
+ std::string to_add_autosuggestion = "";
+ m_suggestion_callback(line, to_add_autosuggestion,
+ m_suggestion_callback_baton);
----------------
This will crash with disabled suggestions (m_suggestion_callback can be null if
the feature is disabled).
================
Comment at: lldb/source/Host/common/Editline.cpp:1017
+ el_insertstr(m_editline, to_add.c_str());
+ return CC_REFRESH;
+ }
----------------
gedatsu217 wrote:
> teemperor wrote:
> > If I understand the only effect this whole code has is to return CC_REFRESH
> > instead of CC_REDISPLAY? If yes, then I think you can just replace the
> > `break` below with `return CC_REFRESH;` which will be much simpler.
> > If yes, then I think you can just replace the break below with return
> > CC_REFRESH; which will be much simpler.
>
> Isn't it "return CC_REDISPLAY", not "CC_REFRESH"? I want to return CC_REFRESH
> only when "to_add" is in "to_add_autosuggestion" (e.g. to_add = b,
> to_add_autosuggestion = "breakpoint").
>
> That is because CC_REDISPLAY deletes the gray-colored autosuggested part,
> while CC_REFRESH leaves it.
>
>
I see. What about just retuning always `CC_REFRESH` here? That should work as
we only add text to the end with a normal completion (which is IIRC that's what
`CC_REFRESH` is for, but
```
lang=c++
case CompletionMode::Normal: {
std::string to_add = completion.GetCompletion();
to_add = to_add.substr(request.GetCursorArgumentPrefix().size());
std::string to_add_autosuggestion = "";
to_add.push_back(' ');
el_insertstr(m_editline, to_add.c_str());
return CC_REFRESH;
}
```
That seems to work for me (and it avoids the crash I pointed out above).
Also my main point here is that this is quite a large change just to change the
return value (and the other tab completions below aren't covered and would need
similar changes if we do this change).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81001/new/
https://reviews.llvm.org/D81001
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
