[ADT] function_ref's constructor is unavailable if the argument is not callable.

[sammccall]

Summary

This allows overload sets containing function_ref arguments to work correctly
Otherwise they're ambiguous as anything "could be" converted to a function_ref.

This matches proposed std::function_ref, absl::function_ref, etc.

[kadircet]

thanks, LGTM!

[dblaikie]

llvm/include/llvm/ADT/STLExtras.h

199–209

This now has two different SFINAE approaches (one in a defaulted template type parameter, one in a defaulted function/ctor parameter) - do they need to be different? Could they be merged in some way to at least use the same/common infrastructure?

[kadircet]

llvm/include/llvm/ADT/STLExtras.h

199–209

i believe this is to prevent creation of function_ref from function_ref, so it is different than what the template type parameter is doing. but I suppose they can be merged.

[dblaikie]

llvm/include/llvm/ADT/STLExtras.h

199–209

Yep, I understand that there are two different things that need to be tested - but if they don't need to be tested in different ways, it'd be good if they were merged to reduce confusion when reading the code (like seeing two variables next to each other with different naming conventions - raises questions about whether the differences are important/intentional/significant in some way the reader doesn't understand, etc).

[sammccall]

llvm/include/llvm/ADT/STLExtras.h

199–209

Good point, I merged them to use the existing default-param technique.

Because the compatible-types condition is complex, I extracted it to a variable template.

[sammccall]

Combine SFINAE conditions into one top-level condition.

Fix a couple of broken usages that no longer compile (in default-args that were
never used, so template instantiation didn't catch them).

[sammccall]

Clang-format

llvm/include/llvm/ADT/STLExtras.h

class function_ref<Ret(Params...)> {
   intptr_t callable;
 
   template<typename Callable>
   static Ret callback_fn(intptr_t callable, Params ...params) {
     return (*reinterpret_cast<Callable*>(callable))(
         std::forward<Params>(params)...);
   }
 
+  template <typename Callable,
+            typename Result =
+                typename std::result_of<Callable(Params...)>::type>
+  static constexpr bool IsCompatible =
+      std::is_void<Ret>::value || std::is_convertible<Result, Ret>::value;
+
 public:
   function_ref() = default;
   function_ref(std::nullptr_t) {}
 
   template <typename Callable>
+  // Only allow this constructor if the object is actually callable
+  // and returns the correct type.
   function_ref(
       Callable &&callable,
       std::enable_if_t<
+          // This is not the copy-constructor.
           !std::is_same<std::remove_cv_t<std::remove_reference_t<Callable>>,
-                        function_ref>::value> * = nullptr)
+                        function_ref>::value &&
+          // Must be callable and return a suitable type.
+          IsCompatible<Callable>> * = nullptr)
       : callback(callback_fn<typename std::remove_reference<Callable>::type>),
         callable(reinterpret_cast<intptr_t>(&callable)) {}
 
   Ret operator()(Params ...params) const {
     return callback(callable, std::forward<Params>(params)...);
   }
 
   explicit operator bool() const { return callback; }

llvm/lib/AsmParser/LLParser.h

namespace llvm {
 
   public:
     LLParser(StringRef F, SourceMgr &SM, SMDiagnostic &Err, Module *M,
              ModuleSummaryIndex *Index, LLVMContext &Context,
              SlotMapping *Slots = nullptr)
         : Context(Context), Lex(F, SM, Err, Context), M(M), Index(Index),
           Slots(Slots), BlockAddressPFS(nullptr) {}
     bool Run(
-        bool UpgradeDebugInfo,
-        DataLayoutCallbackTy DataLayoutCallback = [](Module *) {});
+        bool UpgradeDebugInfo, DataLayoutCallbackTy DataLayoutCallback =
+                                   [](StringRef) { return None; });
 
     bool parseStandaloneConstantValue(Constant *&C, const SlotMapping *Slots);
 
     bool parseTypeAtBeginning(Type *&Ty, unsigned &Read,
                               const SlotMapping *Slots);
 
     LLVMContext &getContext() { return Context; }
 

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

public:
   Error materialize(GlobalValue *GV) override;
   Error materializeModule() override;
   std::vector<StructType *> getIdentifiedStructTypes() const override;
 
   /// Main interface to parsing a bitcode buffer.
   /// \returns true if an error occurred.
   Error parseBitcodeInto(
       Module *M, bool ShouldLazyLoadMetadata = false, bool IsImporting = false,
-      DataLayoutCallbackTy DataLayoutCallback = [](std::string) {
-        return None;
-      });
+      DataLayoutCallbackTy DataLayoutCallback = [](StringRef) { return None; });
 
   static uint64_t decodeSignRotatedValue(uint64_t V);
 
   /// Materialize any deferred Metadata block.
   Error materializeMetadata() override;
 
   void setStripDebugInfo() override;
 

llvm/unittests/ADT/FunctionRefTest.cpp

TEST(FunctionRefTest, BadCopy) {
   function_ref<int()> X;
   function_ref<int()> Y = A;
   function_ref<int()> Z = static_cast<const function_ref<int()> &&>(Y);
   X = Z;
   Y = nullptr;
   ASSERT_EQ(1, X());
 }
 
+// Test that overloads on function_refs are resolved as expected.
+const char *returns(StringRef) { return "not a function"; }
+const char *returns(function_ref<double()> F) { return "number"; }
+const char *returns(function_ref<StringRef()> F) { return "string"; }
+
+TEST(FunctionRefTest, SFINAE) {
+  EXPECT_EQ("not a function", returns("boo!"));
+  EXPECT_EQ("number", returns([] { return 42; }));
+  EXPECT_EQ("string", returns([] { return "hello"; }));
+}
+
 } // namespace