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

[Louis Dionne via Phabricator]

ldionne added inline comments.


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


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


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



Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045