[Lldb-commits] [PATCH] D72953: Fix the handling of unnamed bit-fields when parsing DWARF

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Very cool. I think we can improve the code quite a bit while we're here.



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2427
       if (attributes.ExtractFormValueAtIndex(i, form_value)) {
+        // AT_data_member_location indicates the byte offset of the
+        // word from the base address of the structure.
----------------
`DW_AT_data_member_location`.
`AT_foo` makes git grep less useful


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2674
 
-              if (anon_field_info.IsValid()) {
+              if (unnamed_field_info.IsValid()) {
+                if (data_bit_offset != UINT64_MAX)
----------------
Can you please add comments explaining what each of these cases handle?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2691
+              } else
+                field_bit_offset = this_field_info.bit_offset;
             }
----------------
Looks like this could benefit from a refactoring with early exits. Can be a follow-up commit though.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2696
+            last_bitfield_info = this_field_info;
+          } else {
+            last_bitfield_info.Clear();
----------------
for example, it's not obvious what happened to end up in this else branch


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:174
+  struct FieldInfo {
     uint64_t bit_size;
     uint64_t bit_offset;
----------------
Not you fault, but: This data structure is a disaster waiting to happen. Instead of having magic sentinel values, should we use an Optional<FieldInfo> that is always valid if it exists? Or should the members be Optionals?


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

https://reviews.llvm.org/D72953