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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I'm not sure whether I'm bothered that this patch handles the other captures for lambda's with captured "this" pretty differently from ones that don't capture "this".  But the method for the ones that don't capture "this" is more straightforward, so maybe that's desirable.

You introduced a handful of API's that take StackFrame *'s but then their callers end up having StackFrameSP's that they have to call "get" on to pass to your new API's.  That doesn't seem desirable.

Other inline comments...



================
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;
----------------
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...


================
Comment at: lldb/source/Expression/Materializer.cpp:777
+  lldb::ValueObjectSP
+  GetValueObject(ExecutionContextScope *scope) const override {
+    return ValueObjectVariable::Create(scope, m_variable_sp);
----------------
Naively it seems like it would be a bad idea to call GetValueObject twice.  We don't want independent ValueObjectVariable shared pointers floating around for the same Entity.  Should this maybe do an `if (!m_variable_sp)` first?


================
Comment at: lldb/source/Expression/Materializer.cpp:819
+
+  bool LocationExpressionIsValid() const override { return true; }
+
----------------
Is this right?  For instance, in the case where the fake "this" Variable in the lambda expression has an invalid expression, all the ValueObjects you try to make (the "real captured this" as well as any other captures that were hanging off the fake "this" would have invalid location expressions.


================
Comment at: lldb/source/Expression/UserExpression.cpp:101
 
-lldb::addr_t UserExpression::GetObjectPointer(lldb::StackFrameSP frame_sp,
-                                              ConstString &object_name,
-                                              Status &err) {
+lldb::ValueObjectSP UserExpression::GetObjectValueObject(
+    StackFrame *frame, ConstString const &object_name, Status &err) {
----------------
Can we think of a better name for this?  Having two instances of Object with different meanings in the same name is confusing?  Even `GetObjectPointerValueObject` would be clearer.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:68
+namespace {
+// Return ValueObject of 'this' if we are currently
+// inside the unnamed structure of a C++ lambda.
----------------
This comment is confusing.  Partly because you refer to an "unnamed structure" which you then look up by name "this".  I think it's clearer to say that clang makes an artificial class whose members are the captures, and makes the lambda look like a method of that class.  Then the captured "this" is a special case of all the captures.

This method also ONLY returns the fake this if the real "this" was captured, which you should say.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1725
+
+  if (ClangExpressionVariable::ParserVars *parser_vars =
+          AddExpressionVariable(context, pt, ut, std::move(valobj))) {
----------------
Is there enough advantage to move-ing the incoming valobj as opposed to just copying the shared pointer?  It's a little weird to have API's that consume one of their incoming arguments.  If that really is worth doing, you should note that you've done that.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:227
+
+  if (auto thisValSP = GetLambdaValueObject(frame)) {
+    uint32_t numChildren = thisValSP->GetNumChildren();
----------------
It's worth noting that you handle lambda's that capture "this" and lambda's that don't capture "this" differently.  In the former case, we promote all the captures to local variables and ignore the fake this.  In the latter case (because GetLambdaValueObject only returns a valid ValueObjectSP if it has a child called "this"), we keep finding the values by implicit lookup in the fake this instead.

I don't think that a problem, no need to use the more complex method just for consistency, but you should note that somewhere.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h:202
+
+  lldb::addr_t GetCppObjectPointer(StackFrame *frame, ConstString &object_name,
+                                   Status &err);
----------------
Why did you make this take a StackFrame *?  It seems to force some other functions to change from StackFrameSP to StackFrame * but the only place it gets called it has a StackFrameSP, and explicitly calls get to make it a pointer.  That seems awkward.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129078