[PATCH] D123576: Support constructing empty function_ref from other callables that can be "empty"

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini created this revision.
mehdi_amini added reviewers: csigg, dexonsmith.
Herald added a project: All.
mehdi_amini requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This fixes a surprising behavior where changing an existing function_ref
to std::function can break some code in a subtle manner. Here is the
pattern:

  void foo(llvm::function_ref<void()) callback) {
    if (callback) callback();
  }
  
  void bar(llvm::function_ref<void()) callback) {
    foo(callback);
    // do something else with callback, accept an empty callback.
  }

Assuming `bar` needs to change to take a std::function (for example
to be able to store it), the code in foo() would crash if `callback`
is a default constructed function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123576

Files:
  llvm/include/llvm/ADT/STLFunctionalExtras.h
  llvm/unittests/ADT/FunctionRefTest.cpp


Index: llvm/unittests/ADT/FunctionRefTest.cpp
===================================================================
--- llvm/unittests/ADT/FunctionRefTest.cpp
+++ llvm/unittests/ADT/FunctionRefTest.cpp
@@ -59,4 +59,11 @@
   EXPECT_EQ("string", returns([] { return "hello"; }));
 }
 
+TEST(FunctionRefTest, std_function) {
+  // Check that function_ref constructed from an empty std::function is empty.
+  std::function<void()> f;
+  function_ref<void()> f_ref(f);
+  ASSERT_FALSE((bool)f_ref);
+}
+
 } // namespace
Index: llvm/include/llvm/ADT/STLFunctionalExtras.h
===================================================================
--- llvm/include/llvm/ADT/STLFunctionalExtras.h
+++ llvm/include/llvm/ADT/STLFunctionalExtras.h
@@ -56,6 +56,8 @@
       // This is not the copy-constructor.
       std::enable_if_t<!std::is_same<remove_cvref_t<Callable>,
                                      function_ref>::value> * = nullptr,
+      // callable is not convertible to bool
+      std::enable_if_t<!std::is_convertible<Callable, bool>::value> * = nullptr,
       // Functor must be callable and return a suitable type.
       std::enable_if_t<std::is_void<Ret>::value ||
                        std::is_convertible<decltype(std::declval<Callable>()(
@@ -64,6 +66,28 @@
       : callback(callback_fn<typename std::remove_reference<Callable>::type>),
         callable(reinterpret_cast<intptr_t>(&callable)) {}
 
+  template <typename Callable>
+  function_ref(
+      Callable &&callable,
+      // This is not the copy-constructor.
+      std::enable_if_t<!std::is_same<remove_cvref_t<Callable>,
+                                     function_ref>::value> * = nullptr,
+      // callable is convertible to bool
+      std::enable_if_t<std::is_convertible<Callable, bool>::value> * = nullptr,
+      // Functor must be callable and return a suitable type.
+      std::enable_if_t<std::is_void<Ret>::value ||
+                       std::is_convertible<decltype(std::declval<Callable>()(
+                                               std::declval<Params>()...)),
+                                           Ret>::value> * = nullptr)
+      : callback(callback_fn<typename std::remove_reference<Callable>::type>),
+        callable(reinterpret_cast<intptr_t>(&callable)) {
+    // Handles construction from an empty callable (for example a default
+    // constructed std::function) and ensure that bool conversion to `false` is
+    // propagated.
+    if (!callable)
+      callback = nullptr;
+  }
+
   Ret operator()(Params ...params) const {
     return callback(callable, std::forward<Params>(params)...);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D123576.422138.patch
Type: text/x-patch
Size: 2615 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220412/a9038ce7/attachment.bin>