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

109

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

110–114

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

110–114

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 AdjustedParamTBase {
+    static_assert(!std::is_reference<T>::value,
+                  "references should be handled by template specialization");
+    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>&>' or
+  // '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 AdjustedParamTBase<T &> { using type = T &; };
+  template <typename T> struct AdjustedParamTBase<T &&> { using type = T &; };
+
+  template <typename T>
+  using AdjustedParamT = typename AdjustedParamTBase<T>::type;
 
   // 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 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;
   };

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 Incomplete;
+template <typename T> class Templated { 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(Templated<Incomplete> &&)>
+      IncompleteArgumentRValueReference;
+  unique_function<void(Templated<Incomplete> &)>
+      IncompleteArgumentLValueReference;
+  unique_function<void(Templated<Incomplete> *)> IncompleteArgumentPointer;
+  unique_function<Templated<Incomplete> &()> IncompleteResultLValueReference;
+  unique_function<Templated<Incomplete> && ()> IncompleteResultRValueReference2;
+  unique_function<Templated<Incomplete> *()> IncompleteResultPointer;
+}
+
 } // anonymous namespace