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

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2393
     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
-    BitfieldInfo &last_field_info) {
+    FieldInfo &last_bitfield_info, FieldInfo &last_field_info) {
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
----------------
teemperor wrote:
> IIUC then those two bitfield info variables are mutually exclusive? I.e., either the last field was bitfield or a normal field? If yes, would it make sense to model this as one FieldInfo variable and have a bool value for differentiating between them? That way we don't have the possibility that we forget to clear one when we set the other.
It was not obvious during the initial refactoring wave but yes, I removed one of them.


================
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)
----------------
aprantl wrote:
> Can you please add comments explaining what each of these cases handle?
I removed a lot of in the next wave of refactoring.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:174
+  struct FieldInfo {
     uint64_t bit_size;
     uint64_t bit_offset;
----------------
aprantl wrote:
> 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?
This is a good point, it took a bit more refactoring of the code using this but I removed the magic numbers and used an `Optional<FieldInfo>` for the `unnamed_field_info` case.


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

https://reviews.llvm.org/D72953