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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Very nice!



================
Comment at: include/lldb/Expression/ExpressionParser.h:52
 
+  virtual bool Complete(StringList &matches, unsigned line, unsigned pos,
+                        unsigned typed_pos) = 0;
----------------
Could you add a Doxygen comment?


================
Comment at: source/Commands/CommandObjectExpression.cpp:333
+  // Skip any leading whitespace.
+  while (!cmd.empty() && cmd.front() == ' ') {
+    cmd = cmd.drop_front();
----------------
Even if it is less efficient, a version of this using cmd.split(' ') or a loop over cmd.ltrim(' ') would probably be much shorter and potentially easier to read. There is also llvm::StringExtras::getToken().


================
Comment at: source/Commands/CommandObjectExpression.cpp:392
+
+  if (target) {
+    std::string arg;
----------------
Could you convert this to an early exit?
```
if (!target)
  return 0;
```


================
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())) {
----------------
Should this perhaps use some form of StringRef::dropWhile or similar?
(https://llvm.org/doxygen/classllvm_1_1StringRef.html#aed7897c6ee084440516a7bb5e79a2094)


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:696
+        // If we have a function decl that has no arguments we want to
+        // complete the empty brackets for the user. If the function has
+        // arguments, we at least complete the opening bracket.
----------------
s/brackets/parentheses/


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:699
+        if (const FunctionDecl *F = dyn_cast<FunctionDecl>(D)) {
+          if (F->getNumParams() == 0) {
+            ToInsert += "()";
----------------
no need for curly braces here


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionParser.h:172
+  ///    The number of parsing errors.
+  //-------------------------------------------------------------------
+  unsigned ParseInternal(DiagnosticManager &diagnostic_manager,
----------------
Perhaps return an llvm::Error instead of an unsigned?


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:629
+  column = 0;
+
+  assert(abs_pos <= code.size() && "Absolute position outside code string?");
----------------
You could also slice the StringRef at abs_pos, ::count('\n'), set column to slice.size-rfind('\n'). Your implementation is slightly faster, tough.


================
Comment at: source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp:659
+  lldb::LanguageType lang_type = lldb::LanguageType::eLanguageTypeUnknown;
+  if (auto new_lang = GetLanguageForExpr(diagnostic_manager, exe_ctx)) {
+    lang_type = new_lang.getValue();
----------------
We don't typically put single statements in curly braces in LLVM coding style.


https://reviews.llvm.org/D48465