[libcxx-commits] [PATCH] D116974: [libc++] Add ranges::in_fun_result

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/in_fun_result.h:16
+#include <__utility/move.h>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
----------------
Does this file need the `#pragma GCC system_header` annotation?


================
Comment at: libcxx/include/__algorithm/in_fun_result.h:22
+namespace ranges {
+template <class _Ip, class _Fp>
+struct in_fun_result {
----------------
Note: in `in_out_result`, I used more descriptive/verbose names like `_InputIterator`. Ideally these should be consistent. I think that having long names is more common in `__algorithm/` (quick grepping shows ~360 hits for `_InputIterator` and 0 hits for `_Ip` in my current working copy).


================
Comment at: libcxx/include/__algorithm/in_fun_result.h:29
+    requires convertible_to<const _Ip&, _I2> && convertible_to<const _Fp&, _F2>
+  constexpr operator in_fun_result<_I2, _F2>() const & {
+    return {in, fun};
----------------
`_LIBCPP_HIDE_FROM_ABI`?


================
Comment at: libcxx/include/algorithm:26
+  template <class I, class F>
+    struct in_fun_result; // since C++20
 }
----------------
Quuxplusone wrote:
> `in_out_result` isn't listed here; and `in_in_out_result` either isn't listed, or merge-conflicts.
`in_out_result` is at the bottom of synopsis (around line 655). I don't have a very strong opinion on where in the synopsis these should be listed, but they should all be together; I'd slightly prefer having them in the end because I feel that these are helper types that don't really need to be in the spotlight.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp:80
+  test();
+  static_assert(test());
+}
----------------
Please add `return 0;`.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:24
 struct Empty {};
 
 static_assert(sizeof(std::ranges::in_in_result<Empty, int>) == sizeof(int));
----------------
Can you please move the corresponding tests in `in_out_result.compile.pass.cpp` to this file for consistency? (not necessarily in this patch)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:31
+static_assert(sizeof(std::ranges::in_fun_result<int, Empty>) == sizeof(int));
+static_assert(sizeof(std::ranges::in_fun_result<Empty, Empty>) == 2 * sizeof(Empty));
----------------
Would it be worthwhile to also test for the case of `Empty` and `Empty2` where both classes are empty but they are distinct types?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116974