[libcxx-commits] [PATCH] D130631: [libc++] Implement P0288R9 (move_only_function)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__config:141-142
 #    define _LIBCPP_ABI_DO_NOT_EXPORT_TO_CHARS_BASE_10
+// Enable clang::trivial_abi for std::move_only_function
+#    define _LIBCPP_ABI_MOVE_ONLY_FUNCTION_TRIVIAL_ABI
 #  elif _LIBCPP_ABI_VERSION == 1
----------------
ldionne wrote:
> Since `move_only_function` is new, why do we need an ABI macro to control this?
> 
> Edit: @philnik tells me that's because GCC doesn't support the attribute. What a shame. Since we want to keep code ABI compatible between Clang and GCC (when compiled using libc++), I guess we do need to hide that.
> 
> @philnik Can you please file a bug against GCC to start supporting the `trivial_abi` attribute and paste a link here? I think this is a pretty serious missed optimization that would be so easy to get. CC @jwakely for awareness.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107187


================
Comment at: libcxx/include/__functional/move_only_function.h:26
+#  define _LIBCPP_MOVE_ONLY_FUNCTION_REF &
+#  include <__functional/move_only_function_impl.h>
+
----------------
lichray wrote:
> philnik wrote:
> > lichray wrote:
> > > We should aim for a macro-free implementation to benefit users.
> > If you have an idea on how to do that I'd be happy to try it, but I don't think writing the implementation 12 times is worth removing the macros.
> No macro and maintainable. Here is an example: https://github.com/zhihaoy/nontype_functional/blob/main/include/std23/move_only_function.h
> With C++ explicit object member function, you can also turn the six `operator()` into one.
I don't think that implementation is conforming. The standard explicitly states that there have to be partial specializations for each combination of cv, ref and noex (http://eel.is/c++draft/func.wrap.move#general-1). I didn't ask before, but how exactly does it affect users whether the implementation is macro-free or not?


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:243
+
+  static constexpr size_t __buffer_size_ = 4 * sizeof(void*);
+
----------------
ldionne wrote:
> IMO this is too large. I believe that types with `sizeof(T) == 32` AND that are trivially move constructible/destructible AND that we want to type erase are going to be quite rare. I think that most types will be either smaller, or will be non-trivial. Shooting from the hip, I would use something like `2*sizeof(void*)` at most, but we definitely need to look at what other implementations are doing, and also what other common utilities like this do (let's do a quick survey in open source libraries, e.g. have Folly or Abseil already shipped something like this, and if so, is there something we can learn from their experience?).
| library | name | size | buffer size |
| Abseil | `absl::any_invocable` | 32 | 16 |
| libstdc++ | `std::move_only_function` | 40 | 24 |
| folly | `folly::Function` | 64 | 48 |
| LLVM | `llvm::unique_function` | 32 | 24 |
| HPX | `hpx::move_only_function` | 40 | 24 |
| MSVC STL | `std::move_only_function` | 64 | 56 |



================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.move/call/normal_const.pass.cpp:83
+    assert(f);
+    LIBCPP_ASSERT(f.__get_status() == std::__move_only_function_storage::_Status::_TriviallyDestructible);
+  }
----------------
lichray wrote:
> We should test against behaviors, not guts. If the difference is not observable according to the standard, consider observing some extension such as a more strict `noexcept`.
What are you talking about?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631