[PATCH] D35373: Use delegation instead of inheritance

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Generally looks good to me - few bits to be tidied up, etc. 
Haven't looked meticulously closely at the bits of code that moved around, but I assume there wasn't anything particularly novel there.

Might be worth waiting for a few more eyes, but doesn't seem too crazy.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:56
+// result is not used.
+class DWARFObj {
+  DWARFSection Dummy;
----------------
Pull this out into a separate file, probably? (& maybe even pull DWARFObjInMemory into a separate file too - we do end up with some pretty big/glommy headers)

(no need to do this pre-emptively, if you want to wait to see how the design choices shake out)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:60
+public:
+  virtual ~DWARFObj() {}
+  virtual StringRef getFileName() const { llvm_unreachable("unimplemented"); }
----------------
= default?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:91-95
+  virtual const DWARFSection &getRangeDWOSection() const { return Dummy; }
+  virtual const DWARFSection &getAddrSection() const { return Dummy; }
+  virtual const DWARFSection &getAppleNamesSection() const { return Dummy; }
+  virtual const DWARFSection &getAppleTypesSection() const { return Dummy; }
+  virtual const DWARFSection &getAppleNamespacesSection() const {
----------------
Given that DWARFSection is now only StringRef - why not return by value & return "" like the rest? (& I sitll don't know whether DWARFSection needs to exist at all if it's only a wrapper around StringRef - it makes this whole class look a bit random/arbitrary between some functions returning StringRef for a section, and others returning const DWARFSection&)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:156
+protected:
+  const DWARFObj &DObj;
+
----------------
Why is this non-owning and DWARFContextInMemory owning?

Are tehre any other implementations of DWARFContext (other than DWARFContextInMemory) or should we just do away with the abstraction if it's unused?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFSection.h:18-24
 struct DWARFSection {
   StringRef Data;
+};
+
+struct DWARFSectionMap final : public DWARFSection {
   RelocAddrMap Relocs;
 };
----------------
What's the reason for this split? Is DWARFSection much use if it's a straight up wrapper around StringRef?


https://reviews.llvm.org/D35373