vsk added inline comments.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:46
+ // Member __f_ has type __base*, the contents of which will either directly
+ // hold a pointer to the callable object or vtable entry which will hold the
+ // type information need to discover the lambda being called.
----------------
'or' -> 'or hold a'
'will hold' -> 'points to'
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:47
+ // hold a pointer to the callable object or vtable entry which will hold the
+ // type information need to discover the lambda being called.
+ ValueObjectSP member__f_(
----------------
'need' -> 'needed'
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:49
+ ValueObjectSP member__f_(
+ valobj_sp->GetChildMemberWithName(ConstString("__f_"), true));
+ lldb::addr_t member__f_pointer_value = member__f_->GetValueAsUnsigned(0);
----------------
Could you explicitly pass in a DynamicValueType into GetChildMemberWithName
instead of true? You should be able to find one from
valobj->GetManager()->GetUseDynamic().
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:55
+
+ // We may need to increment pointers into __f_ so we need to know the size
+ uint32_t address_size = process->GetAddressByteSize();
----------------
The address size is a generically useful thing to have around, there's no need
to comment about defining this.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:58
+
+ if (process != nullptr) {
+ Status status;
----------------
Please use an early exit here, and everywhere else in the function where it's
possible to do so. If the process has really disappeared, there's no need to
print anything. In general there's no need to present an incomplete result from
this formatter.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:71
+ lldb::addr_t address_after_vtable = member__f_pointer_value + address_size;
+ lldb::addr_t possible_function_address =
+ process->ReadPointerFromMemory(address_after_vtable, status);
----------------
When you say "possible", what do you mean? Could you add a comment?
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:72
+ lldb::addr_t possible_function_address =
+ process->ReadPointerFromMemory(address_after_vtable, status);
+
----------------
Do you need to check `status.Fail()` here?
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:76
+
+ if (!target.GetSectionLoadList().IsEmpty()) {
+ Address vtable_addr_resolved;
----------------
Before doing anything related to SectionLoadList, it would help to have a short
comment explaining the intent of the code. Looking through it, it seems like
it'd be even better if split out into a helper function.
================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:80
+ Symbol *symbol;
+
+ if (target.GetSectionLoadList().ResolveLoadAddress(
----------------
Instead of attempting to parse symbol names, which is inherently brittle and
complicated, why not get the base address of the callable and look up it's
location via the symbol table? You'd lose some precision in being able to
disambiguate lambdas vs. regular functions, but that's a great tradeoff to
make, because the code would be simpler and more reliable.
https://reviews.llvm.org/D50864
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
