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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

We need to mark this paper as implemented in the status page. The Status pages haven't been updated for yesterday's meeting yet - let me do that now and then you can rebase on top.



================
Comment at: libcxx/test/std/containers/views/trivially_copyable.compile.pass.cpp:8
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+
----------------
jloser wrote:
> Mordante wrote:
> > A bit pedantic, but this is only required in C++23 not in C++20. Can you add a comment. (P2251 was not retroactively applied.)
> I can add a comment about it just working or mark it as `UNSUPPORTED: c++20`. Thanks for pointing out it doesn't retroactively apply to other standards modes. I don't feel strongly.
> 
> @ldionne @Quuxplusone - do either of you have strong opinions?
I would do

```
// UNSUPPORTED: c++03, c++11, c++14, c++17

// P2251 is supported by libc++ even in C++20 mode.
// UNSUPPORTED: !stdlib=libc++ && c++20
```


================
Comment at: libcxx/test/std/strings/string.view/trivially_copyable.compile.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+// <string_view>
----------------
Can you add a comment explaining what this test is testing? Include a link to P2251.


================
Comment at: libcxx/test/std/strings/string.view/trivially_copyable.compile.pass.cpp:9
+
+// <string_view>
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111197