[PATCH] Factor the Unit section parsing into the DWARFUnitSection class.

Frederic Riss friss at apple.com
Mon Sep 29 12:42:28 PDT 2014


================
Comment at: lib/DebugInfo/DWARFUnit.h:65
@@ -63,1 +64,3 @@
 
+  void parse(DWARFContext &Context, const DWARFDebugAbbrev *DA,
+             StringRef Section, StringRef RS, StringRef SS, StringRef SOS,
----------------
samsonov wrote:
> Can't you fetch all the sections' contents (debug_abbrev, debug_ranges, debug_addr etc.) by using a "Context" argument?
Not trivially, because the sections are different between dwo and normal sections. Would you prefer that the patch implements parse() and parseDWO() that fetch the sections themselves (and have the current parse() become a private parseImpl() method)?

================
Comment at: lib/DebugInfo/DWARFUnit.h:68
@@ +67,3 @@
+             StringRef AOS, const RelocAddrMap *M, bool LE) {
+    if (Parsed)
+      return;
----------------
dblaikie wrote:
> (non-actionable gripe: I'm not a huge fan of the extra flag here (not that it's any worse than the previous check - and it's marginally better in some ways)... calling parse repeatedly only to have it no-op because it's already been parsed does feel a bit weird.
> 
> What would happen if the users instead had an Optional<DWARFUnitSection> and would check that, then construct a DWARFUnitSection passing in the various sections, etc - so there was no possibility of a non-parsed DWARFUnitSection. They were constructed with the data and parsed during construction only?
> 
> (OK, maybe semi-actionable, but could just be fanciful ideas on my part))
You have already mentioned it and I didn't forget it. I too find that the added flag looks clumsy, and for an NFC change, I could have just left the old check as it was (but I really dislike the previous empty() check).

As you point out, doing it with Optional would mean removing the laziness, or rather moving it into another accessor. This looks orthogonal and big enough to mandate another patch if it is wanted. I can look into it in a followup patch.

http://reviews.llvm.org/D5522






More information about the llvm-commits mailing list