teemperor added a comment.
Added some comments on the new code. Also we should be able to see these spaces
in the pexpect output, so I guess we should be able to test this too? To make
this special case less fragile to test, you can make a new subtest in the Test
case by just defining a new function:
@skipIfAsan
@skipIfEditlineSupportMissing
def test_hidden_autosuggestion(self):
@skipIfAsan
@skipIfEditlineSupportMissing
def test_autosuggestion(self):
self.launch(extra_args=["-o", "settings set show-autosuggestion true",
"-o", "settings set use-color true"])
self.expect("help frame v")
self.expect("help frame info")
[type 'help frame v' and check for the three space after the grey part to
cover up the "nfo" text]
So, I *believe* that everything that we want to have in this patch has been
addressed? There are quite a few comments in this thread so it's hard to say if
there are open issues. I played around with the current patch for a bit and I
couldn't find anymore issues, so I think this is in pretty good shape in any
case.
Let's see if @labath or @JDevlieghere have any more comments, otherwise I would
say this can go in and the rest can be fixed as follow up commits.
================
Comment at: lldb/include/lldb/Host/Editline.h:377
+ void *m_suggestion_callback_baton = nullptr;
+ int m_previous_autosuggestion_size = 0;
std::mutex m_output_mutex;
----------------
This probably should be `std::size_t` type (a negative previous suggestion size
seems weird). Also add a comment that this is the length of the previous
autosuggestion + user input in bytes.
================
Comment at: lldb/source/Host/common/Editline.cpp:1071
+ (int)to_add.getValue().length();
+ if (spaces_to_print > 0) {
+ std::string spaces = std::string(spaces_to_print, ' ');
----------------
Whoever sees this in the future will really wonder why we print a bunch of
spaces here, so I think a comment like "Print spaces to hide any remains of a
previous longer autosuggestion".
================
Comment at: lldb/source/Host/common/Editline.cpp:1504
#endif
+ if ((int)line.length() > m_previous_autosuggestion_size)
+ m_previous_autosuggestion_size = (int)line.length();
----------------
I don't think the value of `m_previous_autosuggestion_size` should only grow
(which is what this `if` is doing), as this way we just keep printing a longer
and longer space buffer over time. Just printing enough to clear the buffer of
the previous completion is enough.
Also you can just do this calculation directly after you calculated `int
spaces_to_print` above. This way this code is closer together which hopefully
makes this easier to understand.
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
