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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
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",
----------------
labath wrote:
> 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.
I don't see how a seeded PRNG is flaky, but I think that test is also not important enough that I really want to push for merging it in. I just removed it.


================
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);
----------------
labath wrote:
> 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.
I added another parent revision that gives us access to the raw command string, which removes all the cmd rebuilding code and fixes the cases you pointed out (which are now also in the test suite). I have to see how we can get rid of the string search for `--` in both the completion/execute, but that's OOS.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:590
+    // to completing the current token.
+    StringRef to_remove = cmd;
+    while (!to_remove.empty() && !IsTokenSeparator(to_remove.back())) {
----------------
aprantl wrote:
> Should this perhaps use some form of StringRef::dropWhile or similar?
> (https://llvm.org/doxygen/classllvm_1_1StringRef.html#aed7897c6ee084440516a7bb5e79a2094)
Most of the functions are for working on the start of the string, but I don't see anything like dropWhile for the end of the string. There is not even a reverse find_if or something like that :(


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h:172
+  ///    The number of parsing errors.
+  //-------------------------------------------------------------------
+  unsigned ParseInternal(DiagnosticManager &diagnostic_manager,
----------------
aprantl wrote:
> Perhaps return an llvm::Error instead of an unsigned?
Agreed. But I'll do that in a follow up revision because I didn't actually write the ParseInternal for this commit. It's just the renamed `Parse` function and added the missing documentation and completion parameters.


https://reviews.llvm.org/D48465