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

[Samuel Benzaquen via Phabricator via llvm-commits]

sbenza added inline comments.


================
Comment at: include/functional:1483
+
+  const _Target& target() const { return __f_.first(); }
+  const _Alloc& allocator() const { return __f_.second(); }
----------------
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.


================
Comment at: include/functional:1773
+  static __invoker __create() {
+    return __invoker(__choose_call<_Fun>(__use_small_storage<_Fun>()));
+  }
----------------
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.


================
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;
----------------
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.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045