[PATCH] D95498: ADT: Add SFINAE to the generic IntrusiveRefCntPtr constructors
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 28 15:12:17 PST 2021
dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.
Thanks for the review! Pushed in 17c584551d573f1693990773e29fbe6b4b6fa4f4 <https://reviews.llvm.org/rG17c584551d573f1693990773e29fbe6b4b6fa4f4>.
================
Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:174-178
+ template <class X,
+ std::enable_if_t<std::is_convertible<X *, T *>::value, bool> = true>
IntrusiveRefCntPtr(IntrusiveRefCntPtr<X> &&S) : Obj(S.get()) {
S.Obj = nullptr;
}
----------------
dblaikie wrote:
> 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.. )
Nice catch; I added that in.
================
Comment at: llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp:100-138
+ struct X {};
+ struct Y : X {};
+ struct Z {};
+ static_assert(std::is_convertible<IntrusiveRefCntPtr<X> &&,
+ IntrusiveRefCntPtr<X>>::value,
+ "X -> X is okay");
+ static_assert(std::is_convertible<const IntrusiveRefCntPtr<X> &,
----------------
dblaikie wrote:
> 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.
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95498/new/
https://reviews.llvm.org/D95498
More information about the llvm-commits
mailing list