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

David Blaikie dblaikie at gmail.com
Mon Sep 29 12:54:14 PDT 2014


================
Comment at: lib/DebugInfo/DWARFUnit.h:68
@@ +67,3 @@
+             StringRef AOS, const RelocAddrMap *M, bool LE) {
+    if (Parsed)
+      return;
----------------
friss wrote:
> 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.
Agreed on all counts.

http://reviews.llvm.org/D5522






More information about the llvm-commits mailing list