[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#2158645 <https://reviews.llvm.org/D81001#2158645>, @teemperor wrote:

> Not sure if there is a good way to test that the text is directly behind the cursor, but we can test that it's there.


I guess that would be done by expecting the appropriate cursor movement command.

>   self.child.expect_exact(faint_color + "p frame" + reset)                
>    

` + "^[[move-cursor-left"+strlen("p frame")` (I didn't look up the exact encoding of that)



================
Comment at: lldb/source/Core/IOHandler.cpp:204
+                     .GetAutoSuggestionForCommand(line))
+    result = res.getValue();
+}
----------------
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))`.


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

https://reviews.llvm.org/D81001