teemperor added a comment. This is already looking quite reasonable, I think we're getting closer to something we can merge.
In D81001#2096702 <https://reviews.llvm.org/D81001#2096702>, @gedatsu217 wrote: > Implementation all ascii characters for TypedCharacter. > Making m_use_autosuggestion in Editline.cpp. (I did not know a way to pass > the bool value of IOHandlerEditline to Editline constructor, so I made a > function, Editline::UseAutosuggestion, in Editline class. If the > autosuggestion is valid, this function is called from IOHandlerEditline > constructor. Is this implementation good?) You could just have just passed it to the constructor call a few lines above your change, but this seems fine (and maybe even better as the constructor already has a lot of arguments). > > >> You can test it by just doing settings set show-autosuggestion true (which >> should activate this) and settings set show-autosuggestion true (which >> should deactivate this). It's fine if LLDB requires a restart to do activate >> this setting (that's to my knowledge a limitation of the current way the >> IOHandlers work). > > "restart" means that excuting "quit" in LLDB and executing "bin/lldb" again > in Terminal? I made m_use_autosuggestion in Editline.cpp and it goes well > when show-autosuggestion is DefaultTrue. However, when show-autosuggestion is > DefaultFalse and executing "settings set show-autosuggestion true", > autosuggestion does not work well. Do you know what causes it? 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. 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). ================ Comment at: lldb/source/Core/IOHandler.cpp:277 } + if (debugger.GetUseAutosuggestion()) + m_editline_up->UseAutosuggestion(); ---------------- If this was a normal setter you could just do `m_editline_up->SetShowAutosuggestion(debugger.GetShowAutosuggestions());`. ================ Comment at: lldb/source/Host/common/Editline.cpp:1009 + if (m_use_autosuggestion) { + int length = (int)(to_add.length()); + if ((int)(m_current_autosuggestion.length()) > length && ---------------- If you make it `size_t` then you also don't need all the casting below. ================ Comment at: lldb/source/Host/common/Editline.cpp:1447 + +bool Editline::UseAutosuggestion() { + m_use_autosuggestion = true; ---------------- This shouldn't return a `bool` and have a normal setter name (`SetShowAutosuggestion`). Also the better name for this setting should be `ShowAutosuggestion` and not `UseAutosuggestion` (the same goes for all other variables/functions in the rest of this patch). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81001/new/ https://reviews.llvm.org/D81001 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
