labath added a comment.
I think this would be a very nice feature for lldb. Thank you for working on
this.
However, I am somewhat worried about how you're hooking the expression
completer into the completion machinery. I think this should be cleaned up
first.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:1
+"""
+Test the lldb command line completion mechanism for the 'expr' command.
----------------
Maybe put the test under the `expression_command` sub-tree?
================
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:177-215
+ def generate_random_expr(self, run_index):
+ """
+ Generates a random expression. run_index seeds the rng, so
+ the output of this method is always the same for the same run_index
value
+ """
+ # Some random tokens we built our expression from.
+ tokens = [".", ",", "(", ")", "{", "}", "foo", "a", "some_expr",
----------------
I don't think a test like this has place in the test suite. It tests a
different thing every time it's run and it will be impossible to debug if it
starts to fail/be flaky on the build bots.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:227
+
+ assert num_matches == 0, "Got matches, but didn't expect any: " +
str(available_completions)
+
----------------
We normally use the unittest2 functions to do assertions
(`self.assertEquals(num_matches, 0)`)
================
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/TestExprCompletion.py:246-264
+ def complete_from_to(self, str_input, pattern):
+ interp = self.dbg.GetCommandInterpreter()
+ match_strings = lldb.SBStringList()
+ num_matches = interp.HandleCompletion(str_input, len(str_input), 0,
-1, match_strings)
+ common_match = match_strings.GetStringAtIndex(0)
+
+ if num_matches == 0:
----------------
We already have this function in TestCompletions.py. We should move it so some
place that we can reuse it instead of reimplementing it.
================
Comment at:
packages/Python/lldbsuite/test/functionalities/expr_completion/main.cpp:1
+#include <string>
+
----------------
The rest of this test is test is nicely self-contained. Could we avoid
std::string here and make it fully hermetic?
================
Comment at: source/Commands/CommandObjectExpression.cpp:419-423
+ // We got the index of the arg and the cursor index inside that arg as
+ // parameters, but for completing the whole expression we need to revert
+ // this back to the absolute offset of the cursor in the whole expression.
+ int absolute_pos =
+ WordPosToAbsolutePos(arg, cursor_index, cursor_char_position);
----------------
Will this work correctly if the expression command contains arguments (`expr
-i0 -- my_expression`)? What about quoted strings (`expr "string with
spaces<TAB>`)?
Have you looked at whether it would be possible to refactor the completion api
to provide the absolute position (it has to exist at some point), instead of
trying to reverse-engineer it here?
I think the problem here is that the completion api has this built-in
assumption that you're only ever going to be completing "parsed" commands,
which is why you get the input as an `Args` array and a two-dimensional cursor
position. "expr" command takes "raw" input, which means it does not go through
the usual word-splitting and getopt parsing. You can see that because
CommandObjectExpression::DoExecute takes a `const char *command`, whereas for
"parsed" commands (e.g. "frame variable") the DoExecute function takes the
argument as `Args &command`.
I think it would make sense to start this by refactoring the completion api to
behave the same way as the "DoExecute" api. For "raw" commands , the
HandleCompletion function would take a raw string + absolute position in that
string, and the parsed commands would get the an `Args` array plus the
two-dimensional position. Without that, you're going to get subtle differences
in how the expression is treated for completion purposes and for actual
evaluation.
https://reviews.llvm.org/D48465
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
