[Lldb-commits] [PATCH] D134849: [LLDB][NativePDB] Fix struct layout when it has anonymous unions.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Looks good. I suggested some changes, which I hope will reduce duplication and better empasize the differences between the various branches in the code. I think I understand the algorithm now, and I believe it is ok, though I can't escape the feeling that this could have been done in a simpler way.



================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:459-469
+      if (parent->kind == Member::Struct) {
+        parent->fields.push_back(std::move(fields.back()));
+        end_offset_map[end_offset].push_back(parent);
+      } else {
+        lldbassert(parent == &record &&
+                   "If parent is union, it must be the top level record.");
+        MemberUP anonymous_struct = std::make_unique<Member>(Member::Struct);
----------------
The current version will create a needless nested struct in case of a union with a single member. I think it should be possible to just insert a single field first, and let it be converted to a struct later -- if it is necessary.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp:471-491
+      if (parent->kind == Member::Struct) {
+        MemberUP anonymous_union = std::make_unique<Member>(Member::Union);
+        for (auto &field : fields) {
+          int64_t bit_size = field->bit_size;
+          anonymous_union->fields.push_back(std::move(field));
+          end_offset_map[offset + bit_size].push_back(
+              anonymous_union->fields.back().get());
----------------
This should be equivalent but shorter, and --  I hope -- more obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134849