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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
