[libcxx-commits] [PATCH] D127570: [libc++][format] Use forwarding references.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the review!



================
Comment at: libcxx/include/__format/format_arg.h:213-214
   _LIBCPP_HIDE_FROM_ABI __basic_format_arg_value(const void* __value) noexcept : __ptr_(__value) {}
-  _LIBCPP_HIDE_FROM_ABI __basic_format_arg_value(__handle __value) noexcept : __handle_(__value) {}
+  _LIBCPP_HIDE_FROM_ABI __basic_format_arg_value(__handle&& __value) noexcept
+      : __handle_(std::forward<__handle>(__value)) {}
 };
----------------
vitaut wrote:
> Why pass `__handle` by rvalue or call forward here? Note that handle is just a pair of pointers so it probably makes more sense to pass it by value as before.
There's something odd, I can remove the references. But when I remove the `std::forward` it breaks. I will investigate this at a later time.


================
Comment at: libcxx/test/std/utilities/format/format.functions/P2418.pass.cpp:31
+struct std::formatter<MoveOnly, CharT> : std::formatter<int, CharT> {
+  auto format(const MoveOnly& m, auto& ctx) -> decltype(ctx.out()) {
+    return std::formatter<int, CharT>::format(m.get(), ctx);
----------------
vitaut wrote:
> `format` should be const.
Good catch! Unfortunately `std::formatter<int, CharT>::format` isn't const qualified yet. That's WIP and will be fixed soon. Instead I'll add a TODO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127570