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

[Eric Fiselier via Phabricator]

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>
_LIBCPP_INLINE_VISIBILITY
static constexpr type_info *__get_typeid() {
#ifndef _LIBCPP_NO_RTTI
  return &typeid(_Tp);
#else
  return nullptr;
#endif
```

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
         assert(f2.target<Ptr>());
-        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`).

Sorry.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55045/new/

https://reviews.llvm.org/D55045