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