[PATCH] D95498: ADT: Add SFINAE to the generic IntrusiveRefCntPtr constructors

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - some optional/separate patch suggestions.



================
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;
   }
----------------
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.. )


================
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> &,
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95498/new/

https://reviews.llvm.org/D95498