[Lldb-commits] [PATCH] D48465: Added initial code completion support for the `expr` command

[Pavel Labath via Phabricator via lldb-commits]

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