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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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 {
----------------
Mordante wrote:
> 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.
FWIW I'm cool with either way, but you can't use just `_O` because we don't do that. You could use `_Out`, I think would be the best option.


================
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>>);
----------------
Mordante wrote:
> Do we need to test this? I don't see a requirement the conversion is `noexcept(false)`.
Good point, libraries are allowed to strengthen `noexcept` at will. Strike this (and similar lines throughout).


================
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);
----------------
Mordante wrote:
> 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`
That's not a `test/libcxx/` issue, though; that's a `REQUIRES: clang` issue! And we handle that by `// XFAIL: msvc` at the top of this file. So I think that's fine.

(The presence of the attribute on the data member itself //is// mandated for every conforming library, even if the MSVC //compiler// ignores the attribute.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512