[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: benchmarks/function.bench.cpp:222
+  ~NonTrivialStruct() {}
+};
+
----------------
jsoyke wrote:
> sbenza wrote:
> > These types already exist above.
> I wanted something slightly different here, I want the compiler to actually generate different call operators, hence the GeneratedFunctor below.
Add a comment of some kind or rename the classes.
Otherwise it can be confusing for a maintainer.


================
Comment at: include/functional:1785
+    template <typename _Fun>
+    static const __policy* __choose_policy(/* is_small = */ std::true_type)
+    {
----------------
jsoyke wrote:
> sbenza wrote:
> > If `_LIBCPP_NO_RTTI`, we could optimize this by having a single global policy for all small objects.
> Is that worth it? I wouldn't think disabling RTTI is common.
No idea. Projects that disable RTTI might like the extra saving?


================
Comment at: include/functional:2236
+        *this = nullptr;
+        function(_VSTD::move(__f)).swap(*this);
     }
----------------
jsoyke wrote:
> This causes a regression in the non-optimized version, presumably because swap() has a lot of conditionals and potential out-of-line calls, I'll fix this if the new code structure looks good otherwise.
If we are going to use the swap trick, then we don't need to do `=nullptr` in the line before.


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


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


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


================
Comment at: include/functional:2030
+        __policy_(__f.__policy_) {
+      if (__policy_->__destroy) {
+          __f.__policy_ = __policy::__create_empty();
----------------
We don't need a branch here.
Also, unconditionally making the moved-from empty can improve optimizations.


================
Comment at: include/functional:2058
+  void swap(__pfunc& __f) {
+    if (&__f == this)
+      return;
----------------
Is this necessary?


================
Comment at: include/functional:2078
+  const _Tp* target() const _NOEXCEPT {
+      if (__policy_->__is_null || typeid(_Tp) != *__policy_->type_info)
+          return 0;
----------------
Shouldn't this be guarded for `_LIBCPP_NO_RTTI`?
You can't use `typeid`


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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D55045