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

[Louis Dionne via Phabricator]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I think we need to use `std::launder` when accessing the function object through the small buffer. This is a problem we seem to have in the current implementation too.



================
Comment at: include/__config:98
 #  define _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+// Eliminate conditionals and pointer indirection from std::function.
+#  define _LIBCPP_ABI_OPTIMIZED_FUNCTION
----------------
Instead, I think this should be described as an "Unstable attempt to provide a more optimized `std::function`" or something along those lines. My understanding is that we're not bound to your current implementation for the new `std::function` -- we should figure out whatever is fastest and use that.


================
Comment at: include/functional:1476
+template<class _Fp, class _Ap, class _Rp, class ..._ArgTypes>
+class __func<_Fp, _Ap, _Rp(_ArgTypes...)> {
+  __compressed_pair<_Fp, _Ap> __f_;
----------------
We already have one of those in `__functional_03`, right? I understand both files can't be included at the same time, but I would consider changing the name to avoid confusion.


================
Comment at: include/functional:1483
+
+  const _Target& target() const { return __f_.first(); }
+  const _Alloc& allocator() const { return __f_.second(); }
----------------
`_LIBCPP_INLINE_VISIBILITY` here and on `allocator()` too. Also, those should be called `__target()` and `__allocator()` since they are internal, no?


================
Comment at: include/functional:1486
+
+  _LIBCPP_INLINE_VISIBILITY
+  explicit __func(_Target&& __f)
----------------
jsoyke wrote:
> Let me know if I'm using this macro correctly. It seems like it should be present on all non-public symbols?
Roughly speaking, it needs to appear on everything that we don't want to export from the dylib. It's a bit more complicated, but that's the general idea.


================
Comment at: include/functional:1535
 template<class _Rp, class ..._ArgTypes>
-class __base<_Rp(_ArgTypes...)>
+class __vfunc<_Rp(_ArgTypes...)>
 {
----------------
jsoyke wrote:
> No strong feelings on any of the class names I've used here. I was thinking vfunc = vtable based and pfunc = policy based, but I'd be fine with something more descriptive as well.
I'm trying to ponder whether changing this name would break the ABI. If someone explicitly instantiates a `std::function` in a shared object with default visibility, I think this is an ABI break because the dylib (if not recompiled) will export the vtable for `__base`, but the application (when recompiled against new headers) will try to find a definition for the vtable of `__vfunc`.

Now, I'm willing to assume that nobody did something like that. But even then, is there a possibility for an ABI break? @EricWF can you think of something?



================
Comment at: include/functional:1672
+// Policy contains information about how to copy, destroy, and move the
+// undelying functor. You can think of it as a vtable of sorts.
+struct __policy
----------------
Typo: underlying


================
Comment at: include/functional:1679
+    // True if this is the null policy (no value).
+    bool __is_null;
+
----------------
I think this can be made `const`.


================
Comment at: include/functional:1682
+    // The target type. May be null if RTTI is disabled.
+    const std::type_info* type_info;
+
----------------
Mangle to `__type_info` or similar. I know it's not necessary because users can't `#define type_info`, but I find it better to be consistent and mangle everything that's not exposed to users.


================
Comment at: include/functional:1773
+  static __invoker __create() {
+    return __invoker(__choose_call<_Fun>(__use_small_storage<_Fun>()));
+  }
----------------
This would be more readable: `__invoker(__use_small_storage<_Fun>() ? __call_small<_Fun> : __call_large<_Fun>)`.


================
Comment at: include/functional:2079
+      if (__policy_->__is_null || typeid(_Tp) != *__policy_->type_info)
+          return 0;
+      if (__policy_->__clone)  // Out of line storage.
----------------
`nullptr`?


================
Comment at: include/functional:2081
+      if (__policy_->__clone)  // Out of line storage.
+          return (const _Tp*)__buf_.__large;
+      else
----------------
`reinterpret_cast`


================
Comment at: include/functional:2170
     _LIBCPP_INLINE_VISIBILITY
-        _LIBCPP_EXPLICIT operator bool() const _NOEXCEPT {return __f_;}
+        _LIBCPP_EXPLICIT operator bool() const _NOEXCEPT {return (bool)(__f_);}
 
----------------
`static_cast<bool>`? Just a matter of style.


================
Comment at: include/functional:2237
 {
-    *this = nullptr;
-    if (__f.__f_ == 0)
-        __f_ = 0;
-    else if ((void *)__f.__f_ == &__f.__buf_)
-    {
-        __f_ = __as_base(&__buf_);
-        __f.__f_->__clone(__f_);
-    }
-    else
-    {
-        __f_ = __f.__f_;
-        __f.__f_ = 0;
+    if (&__f != this) {
+        *this = nullptr;
----------------
We didn't check for that before, did we? IOW the implementation worked transparently even in the case of a self move-assignment. Would it be possible to retain that property (and save this check in the current implementation)?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045