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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
