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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a reviewer: LLDB.
teemperor added a subscriber: lldb-commits.
teemperor added a comment.

I think this is ready to get a review from the rest. I'll add the other LLDB folks to the review.



================
Comment at: lldb/include/lldb/Host/Editline.h:377
   void *m_completion_callback_baton = nullptr;
+  std::string m_add_completion = "";
 
----------------
`m_current_autosuggestion` (completions are the thing we do when the user presses tab).


================
Comment at: lldb/include/lldb/Interpreter/CommandInterpreter.h:353
 
+  void HandleSuggestion(llvm::StringRef line, std::string &result);
+
----------------
Some documentation maybe:
```
lang=c++
/// Returns the auto-suggestion string that should be added to the given command line.
```


================
Comment at: lldb/source/Core/CoreProperties.td:136
+    Global,
+    DefaultTrue,
+    Desc<"Whether to show autosuggestions or not.">;
----------------
This should be off by default (until the feature is mature somewhen in the future enough to be on by default)/


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


================
Comment at: lldb/source/Core/Debugger.cpp:349
 
+bool Debugger::GetUseAutosuggestion() const {
+  const uint32_t idx = ePropertyShowAutosuggestion;
----------------
You declared the function, but you don't use it anywhere. You should move all the code you added behind `if (GetShowAutosuggestion())` so that it is only active if the user activated the setting (by doing `settings set show-autosuggestion true`).


================
Comment at: lldb/source/Host/common/Editline.cpp:1244
+    llvm::StringRef indent_chars =
+        "abcdefghijklmnopqrstuvwxzyABCDEFGHIJKLMNOPQRSTUVWXZY1234567890 ";
+    for (char c : indent_chars) {
----------------
`.-/()[]{};\"'\\!@#$%^&*_` are missing here. Maybe we should just iterate over all ASCII characters and add all printables except newline or something like that to the alphabet.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1869
 
+void CommandInterpreter::HandleSuggestion(llvm::StringRef line,
+                                          std::string &result) {
----------------
gedatsu217 wrote:
> teemperor wrote:
> > This could just return a `std::string`. Even better, it could return a `llvm::Optional<std::string>` to better mark when no suggestion could be found (and you can return `return llvm::None` to return an empty llvm::Optional value to indicate no return value)
> Does this mean that this function should return llvm::Optional<std::string> instead of void? I do not know what the intent is.
> I personally think either way makes sense.
It just makes the API hard to use incorrectly.

E.g., without knowing how HandleSuggestion is implemented, which of the following calls is correct or incorrect?

```
std::string line = GetLine();
std::string result;
interpreter->HandleSuggestion(line, result); // is this correct?
interpreter->HandleSuggestion(result, line); // or is this correct?
```

Versus:

```
std::string line = GetLine();
std::string result = interpreter->HandleSuggestion(line); // can't call this function in the wrong way
```

The `llvm::Optional` would make it more obvious what the function returns in case there is no suggestion.

I think a name like "GetAutoSuggestionForCommand" or something like that would make it more obvious what this is doing.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1875
+    if (entry.startswith(line)) {
+      auto res = entry.substr(line.size());
+      result = res.str();
----------------
`auto` -> `llvm::StringRef` (it's short enough of a name).


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

https://reviews.llvm.org/D81001