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

[Jordan Soyke via Phabricator]

jsoyke added a comment.

Some tests are failing at this new version. Some seem because the tests depend on under-specified behavior, some might be legitimate so I'll dig into those more tomorrow.



================
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_;
----------------
ldionne wrote:
> 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.
I reverted names of existing classes due to ABI concerns mentioned elsewhere.


================
Comment at: include/functional:1483
+
+  const _Target& target() const { return __f_.first(); }
+  const _Alloc& allocator() const { return __f_.second(); }
----------------
sbenza wrote:
> ldionne wrote:
> > sbenza wrote:
> > > ldionne wrote:
> > > > `_LIBCPP_INLINE_VISIBILITY` here and on `allocator()` too. Also, those should be called `__target()` and `__allocator()` since they are internal, no?
> > > I think catching these unreserved names should be done with tooling.
> > > It is much easier for a ClangTidy tool to detect all incorrect declarations of unreserved names.
> > Technically, `allocator` and `target` are "reserved" because they are used elsewhere in the library. If a user were to `#define` them, they'd be in trouble already.
> > 
> > For that reason, a Clang Tidy check would have to know the interface of everything in the standard library, and enforce that anything not part of such an interface in namespace `std` be mangled. A tool like this can definitely be written, but it may be tedious and difficult to keep up to date. I could be missing another (simpler) solution to implement this, though.
> Yeah, I was thinking of a tool that has a whitelist of identifier names and anything else should be a reserved identifier.
I wonder if parsing the synopsis comments would be reasonable? Anyway, it sounds like there is nothing to change other than visibility here since the name is already "reserved"?


================
Comment at: include/functional:1486
+
+  _LIBCPP_INLINE_VISIBILITY
+  explicit __func(_Target&& __f)
----------------
ldionne wrote:
> 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.
Think I got them all...


================
Comment at: include/functional:1535
 template<class _Rp, class ..._ArgTypes>
-class __base<_Rp(_ArgTypes...)>
+class __vfunc<_Rp(_ArgTypes...)>
 {
----------------
ldionne wrote:
> 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?
> 
Renamed back to the original class name in case this is an issue.

Would be fine since the ctor (which initializes the vtable pointer) is inline? Seems like everything will get folded into the std::function ctor, which should produce a layout compatible object that only references static symbols.

I'd slightly prefer changing the names to be more descriptive now that there are many classes that do the same job, but your concern makes sense so I'll wait till we have a clear answer about this.


================
Comment at: include/functional:1773
+  static __invoker __create() {
+    return __invoker(__choose_call<_Fun>(__use_small_storage<_Fun>()));
+  }
----------------
sbenza wrote:
> ldionne wrote:
> > sbenza wrote:
> > > ldionne wrote:
> > > > This would be more readable: `__invoker(__use_small_storage<_Fun>() ? __call_small<_Fun> : __call_large<_Fun>)`.
> > > This would instantiate both functions when only one is needed.
> > I know, but what's the concern here? Compile-times or code bloat? If it's compile-time, we are instantiating a function just to do the dispatching. If it's code bloat, I'd be _very_ surprised if both definitions actually made it into the final executable, since one of them is obviously not used.
> > 
> > This is not a blocking comment, but I'm not sure we're optimizing for the right thing here. Readability _is_ important, even in a standard library.
> tbh, I think we can make the function be like:
> 
> ```
>   template <typename _Fun>
>   static _Rp __call_impl(const __storage* __buf, _ArgTypes&&... __args)
>   {
>     _Fun* __f = reinterpret_cast<_Fun*>(
>         __use_small_storage<_Fun>::value ? __buf->__large : &__buf->__small);
>     return (*__f)(_VSTD::forward<_ArgTypes>(__args)...);
>   }
> ```
> 
> Then there is a single function that handles both.
Thanks, that's way cleaner.


================
Comment at: include/functional:2081
+      if (__policy_->__clone)  // Out of line storage.
+          return (const _Tp*)__buf_.__large;
+      else
----------------
ldionne wrote:
> `reinterpret_cast`
FWIW, this ends up casting 'const void*' to 'void (*)()' in some cases, which I think is undefined, but the existing implementation relies on this so I didn't worry about fixing it here.


================
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;
----------------
sbenza wrote:
> ldionne wrote:
> > sbenza wrote:
> > > ldionne wrote:
> > > > 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)?
> > > Without this check the code is equally correct, as in the moved-from value has a valid-but-unspecified state. The lhs happens to also be the moved-from value, but that is fine.
> > Rephrasing to make sure I understand: If `this == &__f`, then after the assignment `__f` is in a valid-but-unspecified state (in this case it is empty). Since `this == &__f`, `*this` is also in the same valid-but-unspecified state.
> > 
> > I don't think this makes sense for self-assignment, because the `lhs` is usually understood to contain the value of the `rhs` after a move-assignment. IOW, the following should hold:
> > 
> > ```
> > T x = ...;
> > T copy = x;
> > T y = std::move(x);
> > assert(y == copy);
> > ```
> > 
> > This is not the case here.
> > 
> The example above does not have any self-assignment (or any assignment).
> 
> My argument is that `y = std::move(y);` leaves `y` in an unspecified state. It is not required to be a noop. This is how it behaves today and it is up to spec.
Self move assignment left the function empty before, preserving that behavior now.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045