Skip to content

Conversation

philnik777
Copy link
Contributor

@philnik777 philnik777 commented Jun 21, 2025

We can use if constexpr and __is_invocable_r to simplify the function implementation a bit.

@philnik777 philnik777 force-pushed the simplify_function2 branch 2 times, most recently from 12de290 to 50bc8ae Compare June 25, 2025 13:47
return;

if (sizeof(_Fun) <= sizeof(__buf_) && is_nothrow_copy_constructible<_Fp>::value) {
__f_ = ::new (std::addressof(__buf_)) _Fun(std::move(__f));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be ::new ((void*)std::addressof(__buf_)) _Fun(std::move(__f)); otherwise users can hijack the call to placement new using an overload like

template <class T>
void* operator new(size_t, T*) = delete;

See https://godbolt.org/z/Wfhes8Mff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Fun is always a library-internal type, so I don't think users are allowed to hijack it.

@philnik777 philnik777 force-pushed the simplify_function2 branch from 50bc8ae to 07a2bed Compare July 16, 2025 09:19
@philnik777 philnik777 marked this pull request as ready for review July 16, 2025 14:28
@philnik777 philnik777 requested a review from a team as a code owner July 16, 2025 14:28
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

We can use if constexpr and __is_invocable_r to simplify the function implementation a bit.


Full diff: https://github.com/llvm/llvm-project/pull/145153.diff

1 Files Affected:

  • (modified) libcxx/include/__functional/function.h (+54-72)
diff --git a/libcxx/include/__functional/function.h b/libcxx/include/__functional/function.h
index dc112ebfd0faa..b305af32c1927 100644
--- a/libcxx/include/__functional/function.h
+++ b/libcxx/include/__functional/function.h
@@ -15,16 +15,14 @@
 #include <__cstddef/nullptr_t.h>
 #include <__exception/exception.h>
 #include <__functional/binary_function.h>
-#include <__functional/invoke.h>
 #include <__functional/unary_function.h>
 #include <__memory/addressof.h>
 #include <__type_traits/aligned_storage.h>
 #include <__type_traits/decay.h>
-#include <__type_traits/is_core_convertible.h>
+#include <__type_traits/invoke.h>
 #include <__type_traits/is_scalar.h>
 #include <__type_traits/is_trivially_constructible.h>
 #include <__type_traits/is_trivially_destructible.h>
-#include <__type_traits/is_void.h>
 #include <__type_traits/strip_signature.h>
 #include <__utility/forward.h>
 #include <__utility/move.h>
@@ -95,29 +93,29 @@ template <class _Rp, class _A1, class _A2>
 struct __maybe_derive_from_binary_function<_Rp(_A1, _A2)> : public __binary_function<_A1, _A2, _Rp> {};
 
 template <class _Fp>
-_LIBCPP_HIDE_FROM_ABI bool __not_null(_Fp const&) {
-  return true;
+_LIBCPP_HIDE_FROM_ABI bool __is_null(_Fp const&) {
+  return false;
 }
 
 template <class _Fp>
-_LIBCPP_HIDE_FROM_ABI bool __not_null(_Fp* __ptr) {
-  return __ptr;
+_LIBCPP_HIDE_FROM_ABI bool __is_null(_Fp* __ptr) {
+  return !__ptr;
 }
 
 template <class _Ret, class _Class>
-_LIBCPP_HIDE_FROM_ABI bool __not_null(_Ret _Class::*__ptr) {
-  return __ptr;
+_LIBCPP_HIDE_FROM_ABI bool __is_null(_Ret _Class::* __ptr) {
+  return !__ptr;
 }
 
 template <class _Fp>
-_LIBCPP_HIDE_FROM_ABI bool __not_null(function<_Fp> const& __f) {
-  return !!__f;
+_LIBCPP_HIDE_FROM_ABI bool __is_null(function<_Fp> const& __f) {
+  return !__f;
 }
 
 #  if __has_extension(blocks)
 template <class _Rp, class... _Args>
-_LIBCPP_HIDE_FROM_ABI bool __not_null(_Rp (^__p)(_Args...)) {
-  return __p;
+_LIBCPP_HIDE_FROM_ABI bool __is_null(_Rp (^__p)(_Args...)) {
+  return !__p;
 }
 #  endif
 
@@ -206,12 +204,13 @@ class __value_func<_Rp(_ArgTypes...)> {
   _LIBCPP_HIDE_FROM_ABI explicit __value_func(_Fp&& __f) : __f_(nullptr) {
     typedef __function::__func<_Fp, _Rp(_ArgTypes...)> _Fun;
 
-    if (__function::__not_null(__f)) {
-      if (sizeof(_Fun) <= sizeof(__buf_) && is_nothrow_copy_constructible<_Fp>::value) {
-        __f_ = ::new (std::addressof(__buf_)) _Fun(std::move(__f));
-      } else {
-        __f_ = new _Fun(std::move(__f));
-      }
+    if (__function::__is_null(__f))
+      return;
+
+    if (sizeof(_Fun) <= sizeof(__buf_) && is_nothrow_copy_constructible<_Fp>::value) {
+      __f_ = ::new (std::addressof(__buf_)) _Fun(std::move(__f));
+    } else {
+      __f_ = new _Fun(std::move(__f));
     }
   }
 
@@ -356,7 +355,31 @@ struct __policy {
   // type.
   template <typename _Fun>
   _LIBCPP_HIDE_FROM_ABI static const __policy* __create() {
-    return __choose_policy<_Fun>(__use_small_storage<_Fun>());
+    if constexpr (__use_small_storage<_Fun>::value) {
+      static constexpr __policy __policy = {
+          nullptr,
+          nullptr,
+          false,
+#  if _LIBCPP_HAS_RTTI
+          &typeid(_Fun)
+#  else
+          nullptr
+#  endif
+      };
+      return &__policy;
+    } else {
+      static constexpr __policy __policy = {
+          std::addressof(__large_clone<_Fun>),
+          std::addressof(__large_destroy<_Fun>),
+          false,
+#  if _LIBCPP_HAS_RTTI
+          &typeid(_Fun)
+#  else
+          nullptr
+#  endif
+      };
+      return &__policy;
+    }
   }
 
   _LIBCPP_HIDE_FROM_ABI static const __policy* __create_empty() {
@@ -384,36 +407,6 @@ struct __policy {
   _LIBCPP_HIDE_FROM_ABI static void __large_destroy(void* __s) {
     delete static_cast<_Fun*>(__s);
   }
-
-  template <typename _Fun>
-  _LIBCPP_HIDE_FROM_ABI static const __policy* __choose_policy(/* is_small = */ false_type) {
-    static constexpr __policy __policy = {
-        std::addressof(__large_clone<_Fun>),
-        std::addressof(__large_destroy<_Fun>),
-        false,
-#  if _LIBCPP_HAS_RTTI
-        &typeid(_Fun)
-#  else
-        nullptr
-#  endif
-    };
-    return &__policy;
-  }
-
-  template <typename _Fun>
-  _LIBCPP_HIDE_FROM_ABI static const __policy* __choose_policy(/* is_small = */ true_type) {
-    static constexpr __policy __policy = {
-        nullptr,
-        nullptr,
-        false,
-#  if _LIBCPP_HAS_RTTI
-        &typeid(_Fun)
-#  else
-        nullptr
-#  endif
-    };
-    return &__policy;
-  }
 };
 
 // Used to choose between perfect forwarding or pass-by-value. Pass-by-value is
@@ -455,14 +448,15 @@ class __policy_func<_Rp(_ArgTypes...)> {
 
   template <class _Fp, __enable_if_t<!is_same<__decay_t<_Fp>, __policy_func>::value, int> = 0>
   _LIBCPP_HIDE_FROM_ABI explicit __policy_func(_Fp&& __f) : __policy_(__policy::__create_empty()) {
-    if (__function::__not_null(__f)) {
-      __func_   = __call_func<_Fp>;
-      __policy_ = __policy::__create<_Fp>();
-      if (__use_small_storage<_Fp>()) {
-        ::new ((void*)&__buf_.__small) _Fp(std::move(__f));
-      } else {
-        __buf_.__large = ::new _Fp(std::move(__f));
-      }
+    if (__function::__is_null(__f))
+      return;
+
+    __func_   = __call_func<_Fp>;
+    __policy_ = __policy::__create<_Fp>();
+    if (__use_small_storage<_Fp>()) {
+      ::new ((void*)&__buf_.__small) _Fp(std::move(__f));
+    } else {
+      __buf_.__large = ::new _Fp(std::move(__f));
     }
   }
 
@@ -615,21 +609,9 @@ class function<_Rp(_ArgTypes...)>
 
   __func __f_;
 
-  template <class _Fp,
-            bool = _And<_IsNotSame<__remove_cvref_t<_Fp>, function>, __is_invocable<_Fp, _ArgTypes...> >::value>
-  struct __callable;
-  template <class _Fp>
-  struct __callable<_Fp, true> {
-    static const bool value =
-        is_void<_Rp>::value || __is_core_convertible<__invoke_result_t<_Fp, _ArgTypes...>, _Rp>::value;
-  };
-  template <class _Fp>
-  struct __callable<_Fp, false> {
-    static const bool value = false;
-  };
-
   template <class _Fp>
-  using _EnableIfLValueCallable _LIBCPP_NODEBUG = __enable_if_t<__callable<_Fp&>::value>;
+  using _EnableIfLValueCallable _LIBCPP_NODEBUG =
+      __enable_if_t<!is_same<__remove_cvref_t<_Fp>, function>::value && __is_invocable_r_v<_Rp, _Fp&, _ArgTypes...>>;
 
 public:
   typedef _Rp result_type;

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch LGTM except for the laziness removal, which I wonder if it could cause problems.

@philnik777 philnik777 merged commit a5b5248 into llvm:main Sep 3, 2025
70 of 78 checks passed
@philnik777 philnik777 deleted the simplify_function2 branch September 3, 2025 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants