[libcxx][test] Verify customization point object properties

[jloser]

Summary

Add test for various customization point object properties as defined by
the Standard. Test various CPOs from <ranges>, <iterator>,
<concepts>, etc.

The test is mostly from https://reviews.llvm.org/D107036 and split up
into this - thanks Arthur!

[jloser]

Fix formatting in customization_point_object.h

[• Quuxplusone]

Seems generally commendable, and will save me from having to touch D107036 again — in fact I'll go abandon it now. :)

Take a look at my test in 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.

libcxx/test/std/ranges/range.access/cpo.compile.pass.cpp

22 ↗

(On Diff #393710)

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. ;)

libcxx/test/support/customization_point_object.h

19 ↗

(On Diff #393710)

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.

[jloser]

Remove union check in customization_point_object definition
Update list of types for range.access. Mark several as TODO as they're not implemented yet. Match the ordering as defined on cppreference.

[ldionne]

I like the addition of the concept, but why not add the tests to the files where we already test those CPOs? It could become just a section of that test, no?

[var-const]

Generally LGTM, but consider adding the new tests either to existing test files or to a single new test file devoted to verifying customization points.

[jloser]

I think I messed up updating this patch. Instead, it created a new revision at https://reviews.llvm.org/D115777 - sorry about that. I've addressed the feedback and decided to devote all the CPOs from the various headers (<concepts>, <iterator>, <ranges>, etc.) into one test file in that new PR. Thanks for pointing me at your tests from https://reviews.llvm.org/D107036, @Quuxplusone!

I just force updated this revision, so I'll abandon the accidentally-opened-new one.

[jloser]

Force update

[jloser]

Remove extra newline

[ldionne]

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

libcxx/test/std/library/description/conventions/customization.point.object/cpo.pass.cpp

41

This main() can be removed, and the test can be made a .compile.pass.cpp test instead.

[jloser]

In D115588#3195937, @ldionne wrote:

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

Thoughts @Quuxplusone @var-const or others?

[ldionne]

In D115588#3195944, @jloser wrote:

In D115588#3195937, @ldionne wrote:

Personally I quite liked the approach where we'd have a helper to check that a function object is a CPO and then we'd check it from each individual CPO test, but this is fine too. IMO this is just a bit easier to forget to update.

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

When writing new CPOs, one rarely start from scratch. I think most people will look at the tests for existing CPOs and base their new tests on that. If they do it, then this test_cpo would nicely be discoverable.

[jloser]

Rename to .compile.pass.cpp

[jloser]

Fix spacing

[• Quuxplusone]

Mainly I'm ambivalent:

I have no particular opinion on whether these tests should be separated [...] or massed together [...E]ither way might make it harder or easier to forget to do that, depending on your personal git-grepping style.

But, another advantage of the massed-together approach is that we can go ahead and add commented-out lines for all the unimplemented CPOs (as @jloser has done). This gives a "TODO" list that is more explicit rather than implicit. Which is good.

[var-const]

I may slightly prefer having the helper and just use it each test, but don't feel strongly. Arguably, for a "new" (not-yet-implemented CPO like the ones commented out here), it's just as easy to forget to call test_cpo (or whatever we call the helper function template). For the existing, but not-yet-implemented CPOs, this one file approach at least is a gentle TODO for "uncomment me when it's implemented". But, you need to know about the file in the first place from git-grepping around.

Thoughts @Quuxplusone @var-const or others?

FWIW, I have a slight preference to the current single-file approach; from the readability perspective, I find "test for common behavior across a range of related types" to be a more natural structuring than "test a single type for a wide range of different unrelated behaviors".

From the discoverability perspective, the one-file-per-class approach is probably slightly better because someone writing a new test will likely base it on an existing test. OTOH, verifying that the tests cover all existing CPOs is much easier with the single-file approach.

[jloser]

Split test_cpo use into their respective individual files.

Move iter_swap.pass.cpp into iterator.cust.swap directory to mirror its iter_move counterpart. Test the cpo there.

[jloser]

We seem to be pretty split on the two approaches. I just updated the patch to see what others things of the approach of testing the CPO in each file. Let me know what you think.

Still have the one-file approach sitting on a branch and we can stick with that instead if desired.

[ldionne]

This is fine, the other approach was fine too. I have a preference for the current one, but my comment was non-blocking so feel free to use whichever approach you prefer.

[jloser]

Back to single cpo.compile.pass.cpp as discussed offline with ldionne. If CI passes still, I'll land this.

[ldionne]

The CI failure seems to be a fluke. LGTM

libcxx/test/std/library/description/conventions/customization.point.object/cpo.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts, libcpp-has-no-incomplete-ranges
+
+// [customization.point.object]
+// [range.adaptor.object] "A range adaptor object is a customization point object..."
+
+#include <compare>
+#include <iterator>
+#include <ranges>
+#include <utility>
+
+// Test for basic properties of C++20 16.3.3.3.6 [customization.point.object].
+template <class CPO, class... Args>
+constexpr bool test(CPO& o, Args&&...) {
+  static_assert(std::is_class_v<CPO>);
+  static_assert(std::is_trivial_v<CPO>);
+
+  auto p = o;
+  using T = decltype(p);
+
+  // The type of a customization point object, ignoring cv-qualifiers, shall model semiregular.
+  static_assert(std::semiregular<T>);
+
+  // The type T of a customization point object, ignoring cv-qualifiers, shall model...
+  static_assert(std::invocable<T&, Args...>);
+  static_assert(std::invocable<const T&, Args...>);
+  static_assert(std::invocable<T, Args...>);
+  static_assert(std::invocable<const T, Args...>);
+
+  return true;
+}
+
+int main(int, char**)
+{
+    int a[10];
+    //int arrays[10][10];
+    //std::pair<int, int> pairs[10];
+
+    // [concept.swappable]
+    static_assert(test(std::ranges::swap, a, a));
+
+    // [iterator.cust]
+    static_assert(test(std::ranges::iter_move, a + 0));
+    static_assert(test(std::ranges::iter_swap, a + 0, a + 1));
+
+    // [cmp.alg]
+    static_assert(test(std::partial_order, 1, 2));
+    static_assert(test(std::strong_order, 1, 2));
+    static_assert(test(std::weak_order, 1, 2));
+    //static_assert(test(std::compare_partial_order_fallback, 1, 2));
+    //static_assert(test(std::compare_strong_order_fallback, 1, 2));
+    //static_assert(test(std::compare_weak_order_fallback, 1, 2));
+
+    // [range.access]
+    static_assert(test(std::ranges::begin, a));
+    static_assert(test(std::ranges::end, a));
+    static_assert(test(std::ranges::cbegin, a));
+    //static_assert(test(std::ranges::cdata, a));
+    static_assert(test(std::ranges::cend, a));
+    //static_assert(test(std::ranges::crbegin, a));
+    //static_assert(test(std::ranges::crend, a));
+    static_assert(test(std::ranges::data, a));
+    static_assert(test(std::ranges::empty, a));
+    //static_assert(test(std::ranges::rbegin, a));
+    //static_assert(test(std::ranges::rend, a));
+    static_assert(test(std::ranges::size, a));
+    static_assert(test(std::ranges::ssize, a));
+
+    // [range.factories]
+    // views::empty<T> is not a CPO
+    static_assert(test(std::views::iota, 1));
+    static_assert(test(std::views::iota, 1, 10));
+    //static_assert(test(std::views::istream<int>, 1);
+    //static_assert(test(std::views::single, 4));
+
+    // [range.adaptors]
+    static_assert(test(std::views::all, a));
+    static_assert(test(std::views::common, a));
+    static_assert(test(std::views::counted, a, 10));
+    //static_assert(test(std::views::drop, a, 10));
+    //static_assert(test(std::views::drop_while, a, [](int x){ return x < 10; }));
+    //static_assert(test(std::views::elements<0>, pairs));
+    //static_assert(test(std::views::filter, a, [](int x){ return x < 10; }));
+    //static_assert(test(std::views::join, arrays));
+    static_assert(test(std::views::reverse, a));
+    //static_assert(test(std::views::split, a, 4));
+    //static_assert(test(std::views::take, a, 10));
+    //static_assert(test(std::views::take_while, a, [](int x){ return x < 10; }));
+    static_assert(test(std::views::transform, a, [](int x){ return x + 1; }));
+
+    return 0;
+}