[Lldb-commits] [PATCH] D81001: [lldb] Display autosuggestion part in gray if there is one possible suggestion

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D81001#2162743 <https://reviews.llvm.org/D81001#2162743>, @gedatsu217 wrote:

> In addition to it, I tried the below code, but it did not go well. ("\x1b[nD" moves the cursor n steps to the left.)
>
>   self.child.send("hel")
>   self.child.expect_exact(faint_color + "p frame" + reset + "\x1b[" + str(len("p frame")) + "D")
>
>
> In the first place, the below code does not go well.
>
>   self.child.send("help frame")
>   self.child.expect_exact("help frame" + "\x1b[0D")
>
>
> "\x1b[0D" means that the cursor does not move. So, I suspect "\x1b[nD" does not function in pexpect. What do you think?


It definitely "functions", but it's possible that lldb/editline output a slightly different sequence with the same effect. For example it might output "\x1b[mG", which would move the cursor to the absolute column `m`. Other ways of achieving the same thing are possible, but they are more convoluted so I don't expect we are using them. Bottom line: You'll need to check what's the sequence that actually gets output.



================
Comment at: lldb/source/Core/IOHandler.cpp:204
+                     .GetAutoSuggestionForCommand(line))
+    result = res.getValue();
+}
----------------
gedatsu217 wrote:
> labath wrote:
> > gedatsu217 wrote:
> > > labath wrote:
> > > > Why does this switch Optional<string> result to a by-ref string argument. We have both code which uses an empty string to signify failure, and code which does that with `None`. Both have their advantages and disadvantages, and I don't have any strong objections to either one, but I certainly don't see an advantage in using both in the same patch.
> > > Sorry, I do not what you mean. Would you explain it more in detail?
> > > 
> > > (If None is returned, it is not assigned to "result". So, only is string assigned to "result". Does this answer your question?)
> > Ok, let me try to phrase that differently:
> > - if `GetAutoSuggestionForCommand` fails, it returns `None`
> > - if `IOHandlerDelegate::IOHandlerSuggestion` fails, it sets a by-ref string argument to `""` (or rather, leaves it as empty).
> > 
> > The two behaviors are different, and it's not clear to me what justifies that difference. It would be much cleaner and clearer if both were using the same convention. Changing conventions part way like this is confusing and forces one to add conversion code. If `IOHandlerSuggestion` was using the same convention, then it's implementation would just be `return io_handler.GetDebugger().GetCommandInterpreter().GetAutoSuggestionForCommand(line))`.
> I understood it.
> Even if I change return of IOHandlerSuggestion from void to llvm::Optional<std::string>, I think I have to convert None to empty string in another function eventually, because the final perpose of these function is to make string for autosuggestion. 
> 
> Therefore, if I change return of CommandInterperter::GetAutoSuggestionForCommand from llvm::Optional<std::string> to just std::string, I think above problem will be solved. What do you think?
Are you sure about that? If I follow the code correctly this eventually gets called in `Editline::ApplyAutosuggestCommand` 
```
  std::string to_add = "";
  m_suggestion_callback(line, to_add, m_suggestion_callback_baton);

  if (to_add.empty())
    return CC_REFRESH;

  el_insertstr(m_editline, to_add.c_str());
  return CC_REDISPLAY;
```
That could be written as:
```
if (Optional<string> to_add = m_suggestion_callback(line, to_add, m_suggestion_callback_baton)) {
  el_insertstr(m_editline, to_add->c_str());
  return CC_REDISPLAY;
}
return CC_REFRESH;
```
which is actually shorter and makes the failure case more explicit.

The call in `Editline::TypedCharacter` could be modified similarly...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81001/new/

https://reviews.llvm.org/D81001