[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/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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117512