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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM!



================
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:
> > 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. 
> > The two Empty types don't share the same address when `sizeof(std::ranges::in_in_out_result<Empty, char, Empty>) == 1`
> 
> Hmm, I don't think the compiler is permitted to lay out the second Empty object (or any object) at offset `1` if the sizeof the entire thing is also `1`. However, I'm also aware that my mental model of `[[no_unique_address]]` is not yet correct, so I'm not //sure// you're wrong.
> At least I think it is safe to keep this assert as `== 2` until/unless we discover a platform where that assertion is incorrect, and then we can revisit it with that additional information. I don't think we should relax the assertion here based on hypotheticals that might never come to pass.
Sounds fair. It's at least very unlikely GCC and Clang change, since the change would be an ABI-break. Once MSVC supports `[[no_unique_address]]` we can evaluate the proper value for their implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512