[PATCH] D55045: Add a version of std::function that includes a few optimizations.

Eric Fiselier via Phabricator reviews at reviews.llvm.org
Mon Dec 10 10:32:26 PST 2018

EricWF added inline comments.

Comment at: include/functional:1876
+                    alignof(_Fun) <= alignof(__policy_storage) &&
+                    _VSTD::is_trivially_copy_constructible<_Fun>::value &&
+                    _VSTD::is_trivially_destructible<_Fun>::value> {};
Nit: The `_VSTD::` is unnecessarily when referencing types.

Comment at: include/functional:1703
+    // The target type. May be null if RTTI is disabled.
+    const std::type_info* const __type_info;
Nit: I don't think the `std::` qualifier is needed.

Comment at: include/functional:1717
+        static const _LIBCPP_CONSTEXPR __policy __policy_ = {nullptr, nullptr,
+                                                             true,
+#ifndef _LIBCPP_NO_RTTI
I wouldn't mind naming these magic constants like:

static const _LIBCPP_CONSTEXPR __policy __policy_ = {
  /* __clone */ nullptr,  /* __destroy */ nullptr, /* __is_null */ true,

Comment at: include/functional:1718
+                                                             true,
+#ifndef _LIBCPP_NO_RTTI
+                                                             &typeid(void)
Small nit. I hate conditional compilation blocks, and would like as few as possible. I would write this as:

template <class _Tp>
static constexpr type_info *__get_typeid() {
  return &typeid(_Tp);
  return nullptr;

and then just use that function everywhere else.

Comment at: test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/copy_move.pass.cpp:162
-        LIBCPP_ASSERT(f.target<Ptr>()); // f is unchanged because the target is small
+        // LIBCPP_ASSERT(f.target<Ptr>()); // f is unchanged because the target is small
EricWF wrote:
> Just remove these asserts. I'm not sure how useful they are.
> I guess it means we're allowing ourselves to break existing code, but that existing code depended on UB.
> I think maybe the asserts were an attempt to establish that the SSO was actually being hit.
Actually I take this back. Hyrums law leads me to believe if we regress this behavior in ABI v1, someone will notice.

I think maybe we should still have these asserts for the old ABI (guarded by `#ifndef _LIBCPP_ABI_OPTIMIZED_FUNCTION`).


  rCXX libc++



More information about the libcxx-commits mailing list