Page MenuHomePhabricator

Remove pimpl class from PassRegistry.
ClosedPublic

Authored by zturner on Jun 11 2014, 5:07 PM.

Details

Summary

Remove pimpl class from PassRegistry.

Since removeRegistrationListener is no longer called during static destruction, we can get rid of the pimpl in PassRegistry.

This should clean up the code somewhat, increase clarity, and also allows us to put the Lock as a member of the class, instead of as a ManagedStatic.

As part of this change, the PassInfo class is moved from PassSupport.h to its own file, to eliminate the otherwise circular header dependency between PassRegistry.h and PassSupport.h

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 10339.Jun 11 2014, 5:07 PM
zturner retitled this revision from to Remove pimpl class from PassRegistry..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: dblaikie, rnk.
zturner set the repository for this revision to rL LLVM.
zturner added a subscriber: Unknown Object (MLST).
zturner updated this object.Jun 11 2014, 5:10 PM
dblaikie accepted this revision.Jun 11 2014, 5:21 PM
dblaikie edited edge metadata.

Looks good to me, please commit! (various optional comments, curiosities, etc)

include/llvm/PassInfo.h
1 ↗(On Diff #10339)

I take it you had to split this out to resolve a dependency that wasn't exposed when everything was pmpld away?

include/llvm/PassRegistry.h
61 ↗(On Diff #10339)

You can remove this now that it's got nothing in it.

lib/IR/PassRegistry.cpp
39 ↗(On Diff #10339)

delete this dtor now that it's not doing anything special?

71 ↗(On Diff #10339)

Oh dynamically conditional ownership... (not your problem, just me being sad)

This revision is now accepted and ready to land.Jun 11 2014, 5:21 PM
zturner added inline comments.Jun 11 2014, 5:28 PM
include/llvm/PassInfo.h
1 ↗(On Diff #10339)

Yes, I actually updated the description on Phab, but it didn't send out an email notification for that. Basically, the pimpl has a unique_ptr<PassInfo>, and PassSupport.h already includes PassRegistry.h, so I couldn't have PassRegistry.h include PassSupport.h to get this class.

Plus, one class-one file is just better when the classes are non-trivial :)

dblaikie added inline comments.Jun 11 2014, 5:31 PM
include/llvm/PassInfo.h
1 ↗(On Diff #10339)

Yep - that's what I figured. Just checking.

In theory if the ctor and dtor are out of line, a forward declaration should still be sufficient for the unique_ptr<PassInfo> member to be declared (if the dtor or ctor are inline, then they'd need to emit unique_ptr's dtor which would need to emit a call to PassInfo's dtor, etc...) but as you say, better to pull it into a separate file anyway, and include it in the right places. Looks good.

zturner closed this revision.Jun 12 2014, 9:14 AM
zturner updated this revision to Diff 10364.

Closed by commit rL210793 (authored by @zturner).