[libcxx-commits] [PATCH] D117512: [libc++][ranges] Add ranges::in_in_out_result

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In general it looks good but some minor issues.



================
Comment at: libcxx/include/__algorithm/in_in_out_result.h:22
+namespace ranges {
+template <class _I1, class _I2, class _O1>
+struct in_in_out_result {
----------------
Nit, since there's only one out I would prefer to remove the `1` for the output. That also matches the wording of the Standard. Same for the rest of this header.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp:50
+                                    std::ranges::in_in_out_result<long, long, long>>);
+static_assert(!std::is_nothrow_convertible_v<std::ranges::in_in_out_result<int, int, int>&,
+                                             std::ranges::in_in_out_result<long, long, long>>);
----------------
Do we need to test this? I don't see a requirement the conversion is `noexcept(false)`.


================
Comment at: libcxx/test/std/algorithms/algorithms.results/no_unique_address.compile.pass.cpp:34
+static_assert(sizeof(std::ranges::in_in_out_result<char, Empty, Empty>) == 2);
+static_assert(sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == 2);
+static_assert(sizeof(std::ranges::in_in_out_result<Empty, Empty, char>) == 2);
----------------
Based on http://eel.is/c++draft/basic.memobj#intro.object-9 I expect 1 to be an valid answer. Actually I wonder how portable these answers are. The Standard states `Two objects … may have the same address` is uses `may` and not `shall`. This makes sense since compilers are allowed to ignore attributes. So I think this test should be moved from its current location to `libcxx/test/libcxx/algorithms/algorithms.results/no_unique_address.compile.pass.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512