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