This is an archive of the discontinued LLVM Phabricator instance.

Use delegation instead of inheritance
ClosedPublic

Authored by rafael on Jul 13 2017, 11:15 AM.

Details

Summary

This changes DwarfContext to delegate to DwarfObj instead of having pure virtual methods.

With this DwarfContextInMemory is a trivial class that uses a new implementation of a DwarfObj which can be in a .cpp file.

This is more verbose than I was hopping for, but does allow lld to just implement a DwarfObj. Let me know if you think this is too verbose and I can subclass DwarfContext in lld instead.

Diff Detail

Event Timeline

rafael created this revision.Jul 13 2017, 11:15 AM

Use dummy values instead of unreachable.

dblaikie edited edge metadata.Jul 13 2017, 12:35 PM

Might be helpful to have a bit of a summary about what's changing here, separate from looking at all the source manually - what operations stayed in DWARFContext{,InMemory} versus those that moved out into DWARFObject?

Do all the operations need wrappers in DWARFContext, or could many of those be removed because they're probably only used internally within DWARFContext & could be replaced with "Obj->thing" instead? (though that's more refactoring/churn, I realize)

include/llvm/DebugInfo/DWARF/DWARFContext.h
56

Why do these all need default implementations? Guessing there's a reason they're not pure virtual? Though it seems like it'd make using this class more difficult if there are implicit contracts about which subset of operations are implemented and which ones are not?

Inline trivial functions.

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.

include/llvm/DebugInfo/DWARF/DWARFContext.h
56

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)

60

default?

91–95

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&)

156

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?

include/llvm/DebugInfo/DWARF/DWARFSection.h
18–24

What's the reason for this split? Is DWARFSection much use if it's a straight up wrapper around StringRef?

Move some code to another header.

Move DWARFContextInMemory to a .cpp file I will completely delete it tomorrow.

grimar added a subscriber: grimar.Jul 14 2017, 1:18 AM

Delete DWARFContextInMemory.

dblaikie accepted this revision.Jul 19 2017, 2:26 PM
dblaikie added inline comments.
include/llvm/DebugInfo/DWARF/DWARFObj.h
24 ↗(On Diff #107377)

I'd wonder about the name - DWARFObject? DWARFObjectSections?

unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp
2186 ↗(On Diff #107377)

"*Obj.get()" can probably be "*Obj"? (similarly above)

This revision is now accepted and ready to land.Jul 19 2017, 2:26 PM
espindola closed this revision.Mar 6 2018, 5:22 PM
espindola added a subscriber: espindola.

308543