[libcxx-commits] [PATCH] D115588: [libcxx][test] Verify customization point object properties

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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

Seems generally commendable, and will save me from having to touch D107036 <https://reviews.llvm.org/D107036> again — in fact I'll go abandon it now. :)

Take a look at my test in D107036 <https://reviews.llvm.org/D107036>, though — particularly lines 32–35. I think this PR can and should do something like those `std::invocable` checks.

I have no particular opinion on whether these tests should be separated under all the different Standard sections like you did, or massed together like I did. The only real advantage of massing them together is that it saves a lot of duplication. Someone will have to add a test for each new CPO/niebloid either way, and either way might make it harder //or// easier to forget to do that, depending on your personal git-grepping style.



================
Comment at: libcxx/test/std/ranges/range.access/cpo.compile.pass.cpp:22
+static_assert(customization_point_object<decltype(std::ranges::empty)>);
+static_assert(customization_point_object<decltype(std::ranges::size)>);
----------------
Also `std::ranges::ssize`; `rbegin` etc; `cdata`. Please triple-check all these long lists to make sure you didn't miss anything.

Please either alphabetize, or match cppreference's order, or something. The ordering of lines 16–19 is throwing me off my game. ;)



================
Comment at: libcxx/test/support/customization_point_object.h:19
+    __is_literal_type(T) &&
+    std::semiregular<std::remove_cv_t<T>>;
+
----------------
I think it's safe to eliminate the `|| is_union_v<T>` here, in the name of sanity. :) //Officially// these things aren't even guaranteed not to be functions, right; but in practice they're 100% definitely classes. Which also means they're 100% definitely //not unions//.
Personally I'd also eliminate the `__is_literal_type(T)` check because it doesn't smell portable, but you're certainly right that these things need to be constexpr-time-initializable.
In D10736, my line 22 checks not only that the CPO type is literal, but more specifically that it is constexpr-copy-constructible (and you could add assignment/swap/whatever to that list, if you wanted). To do that kind of thing you need a constexpr function, not a concept.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115588