[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 comment.

In D81001#2107102 <https://reviews.llvm.org/D81001#2107102>, @gedatsu217 wrote:

> Change the name and return of the function( bool UseAutosuggestion() -> void SetShowAutosuggestion (bool) ) (ll. 1447 in Editline.cpp and ll.194 in Editline.h).
>  int -> size_t (ll. 1009 in Editline.cpp).
>  Fix for normal setter (ll. 269 in IOHandler.cpp).
>
> > The Editline instance isn't recreated when you change the settings, so you need to restart LLDB for this to work. However, settings aren't automatically saved, so I think the right way to do this by putting the settings set show-autosuggestion true into the ~/.lldbinit file (which will be automatically executed before LLDB's command line frontend starts). It probably requires a bunch of work to get that working without a restart, so I think this can be fixed as a follow-up.
>
> There is no ~/.lldbinit in my environment (I do not why). Instead, there is ~/.lldb directory (but there are just command history file there.).


You need to create that file :) It's just a text file with LLDB commands in each line (and you just put the settings command in there to change the setting before LLDB starts). Doing `lldb -o "settings set show-autosuggestion true"` should also work.

>> On a more general note: I'm not sure why we need m_current_autosuggestion. There is a bunch of code that tries to keep that variable up-to-date with what is typed, but unless I'm missing something this is just the text the user has entered so far? If yes, then you can also just get the current user input from Editline (see the first few lines of the Editline::TabCommand function for how to do that).
> 
> I think "m_current_autosuggestion" is needed. That is because it keeps the characters for the suggestion, not just user input. For example, when "b" is typed, "reakpoint" is saved in m_current_autosuggestion (when "breakpoint" is typed before).
> 
> When a character is typed, Editline::TypedCharacter is called and m_current_autosuggestion is renewed every time. On the other hand, Editline::ApplyCompleteCommand, which execute suggestion actually, is not called unless C^f is typed. Therefore I think the suggestion parts should be saved until it is called. Indeed, I can get current user input by the first few lines of the Editline::TabCommand function, but it cannot save the suggestion parts probably.
> 
> However, I noticed that "to_add" in Editline::TypedCharacter is unnecessary, so removeed it.

My worry is that if we rely on always keeping this variable up-to-date during the life-time of Editline that this might lead to weird bugs due to the variable getting out-of-sync (e.g., someone changes some input logic but forgets to update the autosuggestion code). I tried moving some parts in this patch around in this commit <https://github.com/Teemperor/llvm-project/compare/arcpatch-D81001...Teemperor:AutosuggestionNoMemberVar> and it does seem to work in the same way as this patch, but without the need to keep m_current_autosuggestion up-to-date. What do you think?



================
Comment at: lldb/source/Host/common/Editline.cpp:1451
+  else
+    m_use_autosuggestion = false;
+}
----------------
This can all be `m_use_autosuggestion = autosuggestion;`.


================
Comment at: lldb/source/Interpreter/CommandInterpreter.cpp:1875-1878
+    if (entry.startswith(line)) {
+      llvm::StringRef res = entry.substr(line.size());
+      result = res.str();
+      return result;
----------------
gedatsu217 wrote:
> labath wrote:
> > ```
> > if (entry.consume_front(line)) {
> >   result = entry.str();
> >   return result; // Why is this returning the result both as a proper return value and a by-ref argument?
> > }
> > ```
> That is because using the llvm::Optional as a return value instead of void would make it more obvious what the function returns in case there is no suggestion.
> 
> We've discussed this issue before, so please see the comments above.
Pavel's point is that the function is returning >both<. You can just remove the `result` parameter and do `result = ... GetAutoSuggestionForCommand(line);` when you call it is his point.


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

https://reviews.llvm.org/D81001