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

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.


================
Comment at: lldb/source/Expression/Materializer.cpp:777
+  lldb::ValueObjectSP
+  GetValueObject(ExecutionContextScope *scope) const override {
+    return ValueObjectVariable::Create(scope, m_variable_sp);
----------------
jingham wrote:
> 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?
`m_variable_sp` is used here to create a new `ValueObjectVariable`. It's a precondition that `m_variable_sp != nullptr` (will add that to the function documentation). I could add a `m_value_object_var` member that we set if it hasn't been set before.


================
Comment at: lldb/source/Expression/Materializer.cpp:819
+
+  bool LocationExpressionIsValid() const override { return true; }
+
----------------
jingham wrote:
> 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.
If the location expression is invalid for the fake "this", would we ever be able to get ValueObject's out of it? By the time these Entity objects get instantiated we either managed to get ValueObject's from "this" or if we didn't, then we wouldn't have added them to the `$__lldb_local_vars` namespace


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1725
+
+  if (ClangExpressionVariable::ParserVars *parser_vars =
+          AddExpressionVariable(context, pt, ut, std::move(valobj))) {
----------------
jingham wrote:
> 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.
Not particularly, don't imagine there's a measurable difference. A copy isn't necessary so I was trying to avoid it. I guess a clearer way would be to just not pass the shared_ptr around, and instead a `ValueObject const&`


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.cpp:227
+
+  if (auto thisValSP = GetLambdaValueObject(frame)) {
+    uint32_t numChildren = thisValSP->GetNumChildren();
----------------
jingham wrote:
> 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.
Good point, will add a comment


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.h:202
+
+  lldb::addr_t GetCppObjectPointer(StackFrame *frame, ConstString &object_name,
+                                   Status &err);
----------------
jingham wrote:
> 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.
Was simply trying to avoid a shared_ptr copy where it wasn't necessary. Arguably the `GetObjectPointer` API should just take a `StackFrame const&`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129078