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

[Shu Anzai via Phabricator via lldb-commits]

gedatsu217 marked an inline comment as done.
gedatsu217 added inline comments.


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


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

https://reviews.llvm.org/D81001