Page MenuHomePhabricator

ADT: Add SFINAE to the generic IntrusiveRefCntPtr constructors
ClosedPublic

Authored by dexonsmith on Jan 26 2021, 7:58 PM.

Details

Summary

Add an enable_if to the generic IntrusiveRefCntPtr constructors so
that std::is_convertible gives an honest answer when the underlying
pointers cannot be converted.

Diff Detail

Event Timeline

dexonsmith created this revision.Jan 26 2021, 7:58 PM
dexonsmith requested review of this revision.Jan 26 2021, 7:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 7:58 PM
dblaikie accepted this revision.Jan 26 2021, 9:51 PM

Looks good - some optional/separate patch suggestions.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
173–178

Can we collapse this ctor with the const ref version by having just one version that accepts by value? (not 100% sure we can - genuinely asking. I think we can because IntrusiveRefCntPtr has real copy ctor/move ctor above, so we won't end up with infinite recursion where the copy ctor needs to use the copy ctor.. )

llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
100–138

Since these are static_asserts, they could just be in the namespace/global scope, rather than being inside a test function. The test function itself does no work when executed, since this is all compile-time checking.

I don't mind too much either way, though.

This revision is now accepted and ready to land.Jan 26 2021, 9:51 PM
This revision was landed with ongoing or failed builds.Jan 28 2021, 3:09 PM
This revision was automatically updated to reflect the committed changes.
dexonsmith marked 2 inline comments as done.Jan 28 2021, 3:12 PM

Thanks for the review! Pushed in 17c584551d573f1693990773e29fbe6b4b6fa4f4.

llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
173–178

Nice catch; I added that in.

llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
100–138

Yeah, it's a bit cleaner that way. I ended up putting them at namespace scope instead. I stripped it down just to the interesting cases (rejecting) and added an actual unit test for the positive cases.