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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/Host/Editline.h:381
   void *m_completion_callback_baton = nullptr;
-
+  std::string m_current_autosuggestion = "";
+  bool m_use_autosuggestion = false;
----------------
Strings are empty by default, so you can omit the assignment: `= ""`. 


================
Comment at: lldb/include/lldb/Host/Editline.h:382
+  std::string m_current_autosuggestion = "";
+  bool m_use_autosuggestion = false;
   std::mutex m_output_mutex;
----------------
Why do we need this? Can we check that the `m_suggestion_callback` is null instead? 


================
Comment at: lldb/include/lldb/Host/Editline.h:320
 
+  unsigned char ApplyCompleteCommand(int ch);
+
----------------
teemperor wrote:
> This function and the one below should be documented like the rest.
It's not really clear to me what this method does based on the name or the description above. It seems similar to `TabCommand`, so maybe `AutosuggestCommand` would be a better name and more consistent? 


================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:355
+  /// command line.
+  llvm::Optional<llvm::StringRef>
+  GetAutoSuggestionForCommand(llvm::StringRef line);
----------------
Although I believe the current implementation is correct (because the returned string is backed by `m_command_history`) I think this interface looks rather suspicious by returning a `StringRef`. Do we envision this returning //new// string in the future? If so we might consider having it return a `llvm::Optional<std::string>`. 


================
Comment at: lldb/source/Core/CoreProperties.td:137
+    DefaultTrue,
+    Desc<"Whether to show autosuggestions or not.">;
 }
----------------
teemperor wrote:
> Usually these settings start like `If true, LLDB will show suggestions on possible commands the user might want to type.` or something like that.
> If true, LLDB will show suggestions to complete the command the user typed.


================
Comment at: lldb/source/Core/IOHandler.cpp:271
     m_editline_up->SetAutoCompleteCallback(AutoCompleteCallback, this);
+    m_editline_up->SetShowAutosuggestion(debugger.GetUseAutosuggestion());
     // See if the delegate supports fixing indentation
----------------
Why should we set the callback if the Autosuggestions are off? Why not do:

```
if(debugger.GetUseAutosuggestion()))
  m_editline_up->SetSuggestionCallback(SuggestionCallback, this);
```

Both have the same downside we already discussed, that the setting cannot be changed for the current EditLine instance. 


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

https://reviews.llvm.org/D81001