[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