[PATCH] Store TypeUnits in a SmallVector<DWARFUnitSection> instead of a single DWARFUnitSection.

David Blaikie dblaikie at gmail.com
Wed Sep 24 10:31:10 PDT 2014


Looks fine to me - some /totally optional/ comments, just if you think they're improvements.

================
Comment at: lib/DebugInfo/DWARFContext.cpp:87
@@ -86,3 +86,3 @@
 
   if ((DumpType == DIDT_All || DumpType == DIDT_Types) && getNumTypeUnits()) {
     OS << "\n.debug_types contents:\n";
----------------
getNumTypeUnits is now going to be potentially misleading - it'll specify the number of type unit sections, not the number of type units.

That happens to be the same for type units (for well constructed DWARF - but again, we might be dealing with bad DWARF when using this tool, so it's interesting to consider the possibility of present though empty type_units sections, etc) but isn't for DWO type units (which, when I first implemented them, went in separate sections - but then I later fixed it per the DWARF Fission spec, to put them all in the same section).

Might be a place where the 'empty(range)' template in that patch I showed previously could be handy - then there's no need for the extra getNum* functions, simply size(dwo_type_unit_sections()) - same behavior, but not quite as misleading.

Honestly though, I'm not sure how much value the "getNum*" calls provide here, just printing the section header seems pretty benign/possibly desired.

Some food for thought. 

================
Comment at: lib/DebugInfo/DWARFContext.cpp:383
@@ -378,1 +382,3 @@
         DataExtractor(I.second.Data, isLittleEndian(), 0);
+    DWOTUs.push_back(DWARFUnitSection<DWARFTypeUnit>());
+    auto &TUS = DWOTUs.back();
----------------
I'm sure I've used this idiom before - I'm not sure if it's nicer than something more like:

  Foo f;
  ...
  v.push_back(std::move(f));

(which I'm sure I've used as well)

================
Comment at: lib/DebugInfo/DWARFContext.h:33
@@ -32,3 +32,3 @@
   DWARFUnitSection<DWARFCompileUnit> CUs;
-  DWARFUnitSection<DWARFTypeUnit> TUs;
+  SmallVector<DWARFUnitSection<DWARFTypeUnit>,1> TUs;
   std::unique_ptr<DWARFDebugAbbrev> Abbrev;
----------------
If it's going to have a small size of 1, maybe just use std::vector?

(granted I'm not quite as averse to the standard containers as some other LLVM contributors/owners - so opinions certainly differ)

================
Comment at: lib/DebugInfo/DWARFContext.h:41
@@ -40,3 +40,3 @@
   DWARFUnitSection<DWARFCompileUnit> DWOCUs;
-  DWARFUnitSection<DWARFTypeUnit> DWOTUs;
+  SmallVector<DWARFUnitSection<DWARFTypeUnit>,1> DWOTUs;
   std::unique_ptr<DWARFDebugAbbrev> AbbrevDWO;
----------------
same here

http://reviews.llvm.org/D5482






More information about the llvm-commits mailing list