Allow incomplete template types in unique_function arguments

[math-fehr]

Summary

We can't declare unique_function that has in its arguments a reference to
a template type with an incomplete argument.
For instance, we can't declare unique_function<void(SmallVectorImpl<A>&)>
when A is forward declared.

This is because SFINAE will trigger a hard error in this case, when instantiating
IsSizeLessThanThresholdT with the incomplete type.

This patch specialize AdjustedParamT for references to remove this error.

[math-fehr]

Update tests and comments

[math-fehr]

Remove unrelated change made by clang-format

[yrouban]

lgtm

llvm/include/llvm/ADT/FunctionExtras.h

92–102

If it was defined as a new template AdjustedParamTBase then the AdjustedParamT could be

template<typename T> using AdjustedParamT = typename AdjustedParamTBase<T>::type;

So we would not need further changes adding '::type' to AdjustedParamT.

[DaniilSuchkov]

llvm/include/llvm/ADT/FunctionExtras.h

93–97

std::is_reference is true not only for l-value references, i.e. if you try adding static_assert(!std::is_reference<T>::value); after this declaration, it'll fail. So it'd be probably better to keep that.
Also please add static_assert(!std::is_lvalue_reference<T>::value); to make the intentions clear.

[DaniilSuchkov]

llvm/include/llvm/ADT/FunctionExtras.h

93–97

So it'd be probably better to keep that.

Probably adding a specialization for T && would be a better idea. In that case I would recommend adding static_assert(!std::is_reference<T>::value); to the original definition (instead of static_assert(!std::is_lvalue_reference<T>::value); recommended above). And a test case with T && would be nice too.

[math-fehr]

Address comments.

• Rename AdjustedParamT to AdjustedParamTBase and define AdjustedParamT alias
• Specialize also for rvalues
• Add static assert to make intentions clear
• Add rvalue tests, and renamed class to better express meaning

[math-fehr]

Thanks, I updated the patch with the comments.
Also, I do not have commit access, so could someone land it for me?

[yrouban]

I'm ok with the patch and will land it. Lets wait for @DaniilSuchkov's resolution.

[DaniilSuchkov]

LGTM

[yrouban]

I have just committed the patch as
https://reviews.llvm.org/rGe3eaff10b29d8bb85a0d09e6bb72fbdb7cc9c3ea
Sorry I forget to add the 'Differential Revision:' line to the comment.

llvm/include/llvm/ADT/FunctionExtras.h

protected:
   // or moves and which are small enough to likely pass in register to values
   // and all other types to l-value reference types. We use this to compute the
   // types used in our erased call utility to minimize copies and moves unless
   // doing so would force things unnecessarily into memory.
   //
   // The heuristic used is related to common ABI register passing conventions.
   // It doesn't have to be exact though, and in one way it is more strict
   // because we want to still be able to observe either moves *or* copies.
-  template <typename T>
-  using AdjustedParamT = typename std::conditional<
-      !std::is_reference<T>::value &&
-          llvm::is_trivially_copy_constructible<T>::value &&
+  template <typename T> struct AdjustedParamT {
+    using type = typename std::conditional<
             llvm::is_trivially_move_constructible<T>::value &&
             IsSizeLessThanThresholdT<T>::value,
         T, T &>::type;
+  };
+
+  // This specialization ensures that 'AdjustedParam<V<T>&>' does not trigger a
+  // compile-time error when 'T' is an incomplete type and V a templated type.
+  template <typename T> struct AdjustedParamT<T &> { using type = T &; };
 
   // The type of the erased function pointer we use as a callback to dispatch to
   // the stored callable when it is trivial to move and destroy.
-  using CallPtrT = ReturnT (*)(void *CallableAddr,
-                               AdjustedParamT<ParamTs>... Params);
+  using CallPtrT = ReturnT (*)(
+      void *CallableAddr, typename AdjustedParamT<ParamTs>::type... Params);
   using MovePtrT = void (*)(void *LHSCallableAddr, void *RHSCallableAddr);
   using DestroyPtrT = void (*)(void *CallableAddr);
 
   /// A struct to hold a single trivial callback with sufficient alignment for
   /// our bitpacking.
   struct alignas(8) TrivialCallback {
     CallPtrT CallPtr;
   };
protected:
 
   bool isInlineStorage() const { return CallbackAndInlineFlag.getInt(); }
 
   bool isTrivialCallback() const {
     return CallbackAndInlineFlag.getPointer().template is<TrivialCallback *>();
   }
 
   CallPtrT getTrivialCallback() const {
-    return CallbackAndInlineFlag.getPointer().template get<TrivialCallback *>()->CallPtr;
+    return CallbackAndInlineFlag.getPointer()
+        .template get<TrivialCallback *>()
+        ->CallPtr;
   }
 
   NonTrivialCallbacks *getNonTrivialCallbacks() const {
     return CallbackAndInlineFlag.getPointer()
         .template get<NonTrivialCallbacks *>();
   }
 
   CallPtrT getCallPtr() const {
protected:
   }
 
   void setOutOfLineStorage(void *Ptr, size_t Size, size_t Alignment) {
     StorageUnion.OutOfLineStorage = {Ptr, Size, Alignment};
   }
 
   template <typename CalledAsT>
   static ReturnT CallImpl(void *CallableAddr,
-                          AdjustedParamT<ParamTs>... Params) {
+                          typename AdjustedParamT<ParamTs>::type... Params) {
     auto &Func = *reinterpret_cast<CalledAsT *>(CallableAddr);
     return Func(std::forward<ParamTs>(Params)...);
   }
 
   template <typename CallableT>
   static void MoveImpl(void *LHSCallableAddr, void *RHSCallableAddr) noexcept {
     new (LHSCallableAddr)
         CallableT(std::move(*reinterpret_cast<CallableT *>(RHSCallableAddr)));

llvm/unittests/ADT/FunctionExtrasTest.cpp

 std::string returns(unique_function<StringRef()> F) { return "string"; }
 
 TEST(UniqueFunctionTest, SFINAE) {
   EXPECT_EQ("not a function", returns("boo!"));
   EXPECT_EQ("number", returns([] { return 42; }));
   EXPECT_EQ("string", returns([] { return "hello"; }));
 }
 
+// A forward declared type, and a templated type.
+class ForwardDeclared;
+template <typename T> class Vec { T a; };
+
+// Check that we can define unique_function that have references to
+// incomplete types, even if those types are templated over an
+// incomplete type.
+TEST(UniqueFunctionTest, IncompleteTypes) {
+  unique_function<void(Vec<ForwardDeclared> &, Vec<ForwardDeclared> *)>
+      IncompleteArguments;
+  unique_function<Vec<ForwardDeclared> &()> IncompleteResultReference;
+  unique_function<Vec<ForwardDeclared> *()> IncompleteResultPointer;
+}
+
 } // anonymous namespace