[libcxx-commits] [PATCH] D111197: [libc++] Verify span and string_view are trivially copyable

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 3 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/test/std/strings/string.view/trivially_copyable.compile.pass.cpp:9
+
+// <string_view>
+
----------------
ldionne wrote:
> jloser wrote:
> > Quuxplusone wrote:
> > > ldionne wrote:
> > > > jloser wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > jloser wrote:
> > > > > > > > Quuxplusone wrote:
> > > > > > > > > This surely needs some `// UNSUPPORTED: c++03, c++11, c++14`, but buildkite will tell you for sure.
> > > > > > > > > ...Actually, it looks like libc++ supports `string_view` as an extension all the way back to C++03? is that true? So you'll want to use `static_assert(std::is_trivially_copyable<...>::value, "");` below.
> > > > > > > > Seems to be the case that `libc++` supports `string_view` all the way back to C++03 oddly enough. Just fixed up the `static_assert`s to play nicely with older modes.
> > > > > > > libc++ originally "back-ported" several features to older standards. This gives us occasionally additional maintenance work. Nowadays we no longer back-port features.
> > > > > > I wonder whether we should disable the test on older versions. This test will fail on MSVC STL in C++11 mode. Looking at cppreference.com I expect other implementations to be trivially copyable from the start.
> > > > > I'd be OK marking this test as unsupported for all standards except C++23.
> > > > Here's what I suggest:
> > > > 
> > > > ```
> > > > // P2251 was voted into C++23, but libc++ guarantees triviality in all Standard modes,
> > > > // so we enable the test in all Standard modes for libc++.
> > > > // UNSUPPORTED: !stdlib=libc++ && (c++03 || c++11 || c++14 || c++17 || c++20)
> > > > ```
> > > > 
> > > > Not the prettiest `UNSUPPORTED` annotation I've seen, but it does the job.
> > > If we really care about `!stdlib=libc++` cases that hypothetically might not have backported this fix, then shouldn't we also start caring about `!stdlib=libc++` cases that don't support `string_view` in C++11 or C++14 at all?
> > > I think we should just be simple and aggressive with both of these tests, and then if someone actually turns up a third-party library that fails these tests, //then// we can PR how to disable them.
> > I like the idea of this being more consistent with our other tests in `libcxx/test` of having these tests work for other standard library implementations. And if one of them fails, then we can mark it as `XFAIL` or `UNSUPPORTED`.  
> > 
> > Thoughts @ldionne?
> Okay, I agree. So I guess we can just remove the `UNSUPPORTED` line entirely then.
Yep, running these tests with all vendors now. Let's see if any fail and go from there now. Thanks @Quuxplusone.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111197