labath added a comment.
The test looks great, and the comments have helped. I still have a couple of
questions about the algorithm though.
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:431
+ uint64_t offset = pair.first;
+ auto &fields = pair.second;
+ lldbassert(offset >= start_offset);
----------------
This shadowing the fields member confused me for quite some time. Could you
choose a different name for one of them?
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459
+ uint64_t end_offset = offset + fields.back()->bit_size;
+ parent->fields.push_back(fields.back());
+ end_offset_map[end_offset].push_back(parent);
----------------
Can `parent` be a union here? Would the algorithm still be correct?
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471
+ } else {
+ for (auto &field : fields) {
+ parent->fields.push_back(field);
----------------
IIUC, this code is reached when the `parent` object is a union. However, the
parent is chosen such that it ends before the start of these new fields?
Therefore its start address will be before the start of these fields as well.
Is it correct to add the fields to the union under these circumstances, or am I
misunderstanding something?
================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h:64
+ uint64_t base_offset;
+ llvm::SmallVector<FieldSP, 1> fields;
+
----------------
zequanwu wrote:
> labath wrote:
> > Can we drop the `SP` part? I think that the owning references (I guess
> > that's this field) could be just plain Field instances (unique_ptr<Field>
> > at worst), and the rest could just be plain pointers and references.
> The Field object is shared between fields and m_fields. And we can't have
> Field member instance inside Field class.
You can't have a `Field` member, but you can have a Field*, unique_ptr<Field>
and std::vector<Field> members. SmallVector<Field> is also not possible, for
reasons that are mostly obvious, but then again, storing a pointer inside a
SmallVector negates most of the benefits that one could hope to gain by using
it.
My point is that that using a shared pointer makes it harder to understand the
relationships between the objects because it obfuscates the ownership aspect.
Sometimes that is unavoidable, like when there just isn't a single object that
can own some other object (although llvm mostly avoids that by putting the
ownership inside some "context" object), but it's not clear to me why that
would be the case here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134849/new/
https://reviews.llvm.org/D134849
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
