[Lldb-commits] [PATCH] D129078: [LLDB][ClangExpression] Allow expression evaluation from within C++ Lambdas

[Adrian Prantl via Phabricator via lldb-commits]

aprantl accepted this revision.
aprantl added a comment.
This revision is now accepted and ready to land.

Nice work! I only had superficial comments, maybe let's also wait for @jingham to accept the patch.



================
Comment at: lldb/include/lldb/Expression/UserExpression.h:283
 
+  static lldb::ValueObjectSP
+  GetObjectPointerValueObject(StackFrame *frame, ConstString const &object_name,
----------------
Nit: The LLVM style wants doxygen comments in the .h file, would be nice to add one here.


================
Comment at: lldb/source/Expression/Materializer.cpp:432
+    m_size = g_default_var_byte_size;
+    m_alignment = g_default_var_alignment;
   }
----------------
FWIW, the refactoring of EntityVariable could have been a separate preparatory NFC patch, then the patch that adds the lambda functionality would be shorter. It's fine either way, but it's usually easier to review two simple comments instead of one complex one :-)


================
Comment at: lldb/source/Expression/Materializer.cpp:783
 
+class EntityVariable : public EntityVariableBase {
+public:
----------------
Maybe add a doxygen comment explaining when this subclass is used as opposed to EntityValueObject? 


================
Comment at: lldb/source/Expression/Materializer.cpp:819
+private:
+  lldb::VariableSP m_variable_sp; ///< Variable that this entity is based on
+  lldb::ValueObjectSP m_value_object_var; ///< ValueObjectVariable created from
----------------
Micro-nit: `.` at the end of sentence.


================
Comment at: lldb/source/Expression/Materializer.cpp:31
 
+static constexpr uint32_t g_default_var_alignment = 8;
+static constexpr uint32_t g_default_var_byte_size = 8;
----------------
jingham wrote:
> This seems weird to me.  You didn't do it, you're just replacing hard-coded 8's later in the code.  But this seems like something we should be getting from the target?  You don't need to solve this for this patch since it isn't intrinsic to it, but at least leave a FIXME...
Yeah this is odd, maybe we can clean this up in a follow-up commit.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:837
+      TypeFromUser pointee_type =
+          capturedThis->GetCompilerType().GetPointeeType();
+
----------------
Nice!


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1151
+
+  if (!variable_found) {
+    if (auto lambda = GetLambdaValueObject(frame)) {
----------------
Maybe comment that this is the lambda/this-capture path?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1700
+  if (ClangExpressionVariable::ParserVars *parser_vars =
+          AddExpressionVariable(context, pt, ut, valobj)) {
+    parser_vars->m_llvm_value = nullptr;
----------------
Nit:

LLVM style would be
```
ClangExpressionVariable::ParserVars *parser_vars =
          AddExpressionVariable(context, pt, ut, valobj);
if (!parser_vars)
  return;
```


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.h:570
 
+  bool GetVariableFromValueObject(CompilerType &comp_type,
+                                  lldb_private::Value &var_location,
----------------
Doxygen comment (unless this is is an override function (but it's not marked as such))?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:222
+    for (uint32_t i = 0; i < numChildren; ++i) {
+      auto childVal = thisValSP->GetChildAtIndex(i, true);
+      ConstString childName(childVal ? childVal->GetName() : ConstString(""));
----------------
at some point we should add a `children()` method that returns a range...


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:306
+
   for (size_t i = 0; i < var_list_sp->GetSize(); i++) {
     lldb::VariableSP var_sp = var_list_sp->GetVariableAtIndex(i);
----------------
not your code, but this one has an iterator :-)


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionUtil.cpp:1
+//===-- ClangExpressionUtil.cpp ---------------------------------*- C++ -*-===//
+//
----------------
the ` -*- C++ -*-` marker only makes sense for `.h` files, where it's ambiguous whether it's a C or a C++ header.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionVariable.h:132
                  /// variable, if it was a symbol
+    lldb::ValueObjectSP m_lldb_value_object; ///< ValueObject for this variable.
+                                             ///< Used when only an ivar is
----------------
Here a `///` comment on top of the declaration might be more readable. (Maybe fix this for the entire class in a follow-up commit)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129078/new/

https://reviews.llvm.org/D129078