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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The test looks wrong to me. If the feature is supposed to be off by default, then how can it test anything without first turning the feature on? Either the feature isn't really off, or the test is broken. I would expect it to be the latter, because the test sequence seems incorrect to me. I would expect to see some newlines in the strings being sent, otherwise all of this will just get concatenated into a single command.

Also, this only seems to test the ^F functionality. It does not check that the command is printed in gray _before_ the user hits ^F.



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


================
Comment at: lldb/source/Host/common/Editline.cpp:1213
+      char bind_key[2] = {0, 0};
+      llvm::StringRef indent_chars =
+          "abcdefghijklmnopqrstuvwxzyABCDEFGHIJKLMNOPQRSTUVWXZY1234567890!\"#$%"
----------------
These don't have anything to do with indenting. Looks like a leftover from the copy paste from line 1255


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

https://reviews.llvm.org/D81001