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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante 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 {
----------------
philnik wrote:
> Quuxplusone wrote:
> > 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.
> I made it `_O1` to be consistent between `in_in_result`, `in_in_out_result` and `in_out_out_result`. I didn't write `in_out_result`, so that one is not consistent.
I think I read there will be some "cleanup" patch to unify all these result types correct?
If so I'm oke postponing this change until that patch.


================
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);
----------------
Quuxplusone wrote:
> 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.)
I agree the adding of the attribute is required. But there's no portable way to check it since compilers are free to ignore attributes. I also am suspicious of the size of this specific object. Both Clang and GCC agree on 2 and due to ABI I don't expect it to change. But I can't see anything preventing another compiler to decide 1 is a nice size in this case. The two `Empty` types don't share the same address when `sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == 1`
Unlike the line above and below where the `Empty`'s are next to each other. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512