This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add utilites for instantiating functions with multiple types
ClosedPublic

Authored by philnik on Nov 4 2022, 7:09 PM.

Details

Summary

We currently call a lot of functions with the same list of types. To avoid forgetting any of them, this patch adds type_lists and utilities for it. Specifically, it adds

  • type_list - This is just a list of types
  • concatenate - This allows concatenating type_lists
  • for_each - Iterate over a type_list

Diff Detail

Event Timeline

philnik created this revision.Nov 4 2022, 7:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 7:09 PM
philnik requested review of this revision.Nov 4 2022, 7:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 7:09 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 473430.Nov 5 2022, 8:33 AM

Try to fix CI

philnik updated this revision to Diff 473458.Nov 5 2022, 1:45 PM

Try to fix CI

EricWF requested changes to this revision.Nov 6 2022, 2:37 PM
EricWF added a subscriber: EricWF.

I don't think the test suite is the place to get clever with code deduplication and template magic.

Readability is significantly more important than repeating yourself. Tests need to be correct upon inspection, because we don't have tests for our tests.
Also I imagine this significantly borks any diagnostics you might get when the test fails.

Tests are not the place to be clever.

This revision now requires changes to proceed.Nov 6 2022, 2:37 PM

I don't think the test suite is the place to get clever with code deduplication and template magic.

Readability is significantly more important than repeating yourself. Tests need to be correct upon inspection, because we don't have tests for our tests.
Also I imagine this significantly borks any diagnostics you might get when the test fails.

Tests are not the place to be clever.

I agree with this and readability is more important. But in certain scenarios, e.g the math tests refactoring Nicolas did which increases the coverage for math tests, it was really hard for me to figure out what types are missing in the original tests as the original tests have hundreds of lines that enumerates all the types it is testing, for every argument, which in my opinion, not very readable. I also think this utility is needed for 3 iterator arguments range algorithms tests , as enumerate the combinations of types make the tests not very readable and hard to spot any combination missing. I think it is a useful tool but requires careful testing of itself

I don't think the test suite is the place to get clever with code deduplication and template magic.

Readability is significantly more important than repeating yourself. Tests need to be correct upon inspection, because we don't have tests for our tests.
Also I imagine this significantly borks any diagnostics you might get when the test fails.

Tests are not the place to be clever.

I agree with this and readability is more important. But in certain scenarios, e.g the math tests refactoring Nicolas did which increases the coverage for math tests, it was really hard for me to figure out what types are missing in the original tests as the original tests have hundreds of lines that enumerates all the types it is testing, for every argument, which in my opinion, not very readable. I also think this utility is needed for 3 iterator arguments range algorithms tests , as enumerate the combinations of types make the tests not very readable and hard to spot any combination missing. I think it is a useful tool but requires careful testing of itself

I agree that tests should be readable. I also agree that the test suite normally isn't the place for template magic. In this case I think the additional complexity is warranted, since it avoids forgetting anything in a list of types that are used commonly throughout the test suite and/or are subject to change over time. This is the case for example for the ranges algorithm tests, where we (should) use specific sets of iterators everywhere. Having the set of types in a central location allows us to update all the tests without being able to forget any one test. The other case are fundamental sets of types, like integral types, which are subject to change over time (i.e. there were additional floating point types added in C++23(?)) and it's a lot harder to miss any tests when all the tests have a single source of truth. Another benefit is that IMO it's a lot easier to understand what you are testing when you say meta::apply_all(meta::integral_types{}, []<class T>() { test<T>(); }). It's instantly clear that you are instantiating the test for all integral types.

As for errors, it doesn't really make them worse. It's just two or three extra entries in the Note: instantiated from here list, which is already miles long in most cases.

I also agree that this magic should be well tested if we want to use it in the tests. If you think I'm missing any coverage I'm happy to add more tests.

This is a major change in our testing infrastructure and I think all the people actively working on libc++ should at least take note of the change (@Mordante @var-const @ldionne - did I forget anyone?).

jloser added a subscriber: jloser.Nov 7 2022, 7:25 PM

I don't think the test suite is the place to get clever with code deduplication and template magic.

Readability is significantly more important than repeating yourself. Tests need to be correct upon inspection, because we don't have tests for our tests.
Also I imagine this significantly borks any diagnostics you might get when the test fails.

Tests are not the place to be clever.

I agree with this and readability is more important. But in certain scenarios, e.g the math tests refactoring Nicolas did which increases the coverage for math tests, it was really hard for me to figure out what types are missing in the original tests as the original tests have hundreds of lines that enumerates all the types it is testing, for every argument, which in my opinion, not very readable. I also think this utility is needed for 3 iterator arguments range algorithms tests , as enumerate the combinations of types make the tests not very readable and hard to spot any combination missing. I think it is a useful tool but requires careful testing of itself

I agree that tests should be readable. I also agree that the test suite normally isn't the place for template magic. In this case I think the additional complexity is warranted, since it avoids forgetting anything in a list of types that are used commonly throughout the test suite and/or are subject to change over time. This is the case for example for the ranges algorithm tests, where we (should) use specific sets of iterators everywhere. Having the set of types in a central location allows us to update all the tests without being able to forget any one test. The other case are fundamental sets of types, like integral types, which are subject to change over time (i.e. there were additional floating point types added in C++23(?)) and it's a lot harder to miss any tests when all the tests have a single source of truth. Another benefit is that IMO it's a lot easier to understand what you are testing when you say meta::apply_all(meta::integral_types{}, []<class T>() { test<T>(); }). It's instantly clear that you are instantiating the test for all integral types.

As for errors, it doesn't really make them worse. It's just two or three extra entries in the Note: instantiated from here list, which is already miles long in most cases.

I also agree that this magic should be well tested if we want to use it in the tests. If you think I'm missing any coverage I'm happy to add more tests.

This is a major change in our testing infrastructure and I think all the people actively working on libc++ should at least take note of the change (@Mordante @var-const @ldionne - did I forget anyone?).

FWIW, I understand the concern around using template metaprogramming adding complexity to the test suite, but I also see the benefits here for easily identifying a family of types and running a test function over that type family. In short, I'm +1 for this.

I too feel we should avoid being "too clever" in unit tests. They should be "considered correct" without trying to do template meta programming in your head. So I agree with @EricWF that some of these things are "too clever". Other parts are already done in other tests. I've seen "test all integral types" tests in older charconv tests. I think it would help to have generic code for "sets of these types". This would avoid typing these lists multiple times with the risk of forgetting a type.

So I like the general idea behind this patch, but we should restrain ourselves against going full template meta programming. I feel you went a bit too far, but I see several improvements too.

Note I didn't do an in depth review, mainly wanted to point out some examples.

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
104

For a unit test this feels too clever for me.

107

This is the kind of cleverness I think is good in unit tests. I still need to look what meta::cpp20_input_iterator_list exact contains, but I usually I won't care, since this is the "set of input iterators we care about". We've done this kind of thing before with "all signed integers".

I really like the consistent use of the namespace meta. Maybe we should do the same for existing test that do this kind of thing.

libcxx/test/std/language.support/support.limits/limits/is_specialized.pass.cpp
47–50

To me this is an improvement. It's clear we want to test all arithmetic types. If we start to support additional arithmetic types it's easy to see what breaks by changing one place. (C++23 will add more floating-point types.)

48

This actually a bug, it should not be available when TEST_HAS_NO_WIDE_CHARACTERS is defined.

libcxx/test/std/strings/basic.string/string.access/at.pass.cpp
79

I consider this an improvement too, here we improve the test coverage.

libcxx/test/support/type_algorithms.h
21

I like the amount of documentation.

135

signed char is a a signed integer type too.

152

This too look a bit clever. Here it's quite obvious, but in a different use-case it might be less obvious.

philnik added inline comments.Nov 10 2022, 10:28 AM
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
104

What exactly do you mean? Concatenating two cartesian products, the caretesian_product_t itself, or maybe something completely different?

libcxx/test/support/type_algorithms.h
135

Good catch!

152

I think in some cases it makes sense, since i.e. if we add another integral type, like __int256_t or something like that, we can't forget to add the unsigned version. The same reasoning applies to the proxy iterator lists. But I agree we should consider whether we use it.

Mordante added inline comments.Nov 10 2022, 10:51 AM
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
104

Mainly the cartesian_product_t. I'm not sure everybody knows what a Cartesian product is. Note that I'm perfectly fine with the Cartesian product in our benchmarks, there it's easy to observe what it does by looking at the output. Here there is not output. So the question is, are you sure the test is correct?

So here I feel writing out the code, using helper functions, makes it easier to understand what happens. Which makes it easier to validate whether the test is correct.

libcxx/test/support/type_algorithms.h
152

Yes so this is a bit the borderline for me. This can be used to avoid errors, but at other places it can make the code harder to understand. It depends on how obvious the effect to the transformation is.

So here I like it. Having well named arguments helps too :-)

philnik added inline comments.Nov 10 2022, 3:24 PM
libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
104

I think most people reading libc++ tests know what a cartesian product is. Even if you don't know what it is, it's about as hard to find out as what a proxy_iterator is, which you also have to know to understand the test. Having the cartesion product spelled out also makes it easier to find out what the intention of the code is. You don't have to go through multiple functions to find out.

ldionne requested changes to this revision.Nov 17 2022, 9:08 AM

I don't think the test suite is the place to get clever with code deduplication and template magic.

I agree. At the same time, tests are also a place where duplication can lead to inconsistent coverage and maintainability issues, when there's as many tests as we have.

I think we need to walk a fine line and find the sweet spot between complexity-because-of-cleverness and complexity-because-of-duplication. After experimenting, we think that 90% of the bang can be achieved with just type_list and for_each, which seems really reasonable.

Readability is significantly more important than repeating yourself.

Again, I agree, however repetition is not necessarily the best way to make code readable. And as we've seen in this review, our tests were sometimes wrong because we had such duplication.

@philnik
I support this direction but I think we can simplify it by providing just for_each and type_list, at least at first. Let's start with that and see where that takes us.

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
98–99

@philnik and I just played around with this, wondering what things would look like if we had a typelist and a foreach-like utility. I think we could get most of the benefits in this patch without incurring to much complexity. This is what this test would look like:

meta::for_each(meta::forward_iterator_list<int*>{}, []<class Out>() {
  test_iterators<cpp20_input_iterator<int*>, Out>();
  test_iterators<ProxyIterator<cpp20_input_iterator<int*>>, Out>();

  meta::for_each(meta::forward_iterator_list<int*>{}, []<class In>() {
    test_iterators<In, Out>();
    test_iterators<In, Out, sized_sentinel<In>>();
    test_iterators<In, Out, sentinel_wrapper<In>>();

    test_iterators<ProxyIterator<In>, Out>();
    test_iterators<ProxyIterator<In>, Out, sized_sentinel<ProxyIterator<In>>>();
    test_iterators<ProxyIterator<In>, Out, sentinel_wrapper<ProxyIterator<In>>>();
  });
});
libcxx/test/std/strings/basic.string/string.access/at.pass.cpp
79

Yeah, I agree -- making it easier to test across wide ranges of types will often increase our coverage because we can easily test everything while still being a bit lazy.

libcxx/test/support/type_algorithms.h
17

I have been thinking about introducing a namespace for our tests utilities for a while. I would have nested this into that namespace.

But I really don't want to start bikeshedding that topic and the namespace name in this review, so I'm happy to go with that and then grep replace it when/if we decide to introduce a namespace for our test utilities.

22–26

This feels a bit loose to me. I would only concatenate type lists together, not arbitrary types.

Otherwise, this breaks "generic programming" (not that we would want to do that in the test suite, but I still think it illustrates why this is surprising). Instead, if we need that functionality, I would call it something like append<list, type>, but I'm not sure we actually need that.

After our experiment, I think we agreed that concatenate and even cartesian_product can go away, at least for now.

51

I don't think that's needed anymore if we don't have cartesian_product.

53–57

How about this? I think it mirrors more closely what it does. It also removes the possibility for confusion between apply and apply_all.

91

I think we're walking a super tight line here. I would suggest removing this helper (even though that will increase duplication a bit when defining the lists below) to avoid opening the door too widely to arbitrary metaprogramming.

131

This seems to be >= C++11, and also char32_t.

philnik updated this revision to Diff 476227.Nov 17 2022, 1:49 PM
philnik marked 16 inline comments as done.

Address commments

@Mordante I think all of your concerns have been addressed now. It would be great if you could comment again whether there is anything you don't like about the current state.
@var-const It would be nice if you could also take a look - you're the only regular contributor that didn't approve the general direction yet (also post-commit in case I land this before you have time to comment).

libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp
107

I think we're all on the same page about adding a namespace for our test utilities. I'll look into adding that. The main problem is probably to find a good name for it, since test is already used everywhere.

libcxx/test/support/type_algorithms.h
22–26

I agreed, but after looking again at the standard I think it makes sense to keep the type_list-concatenating parts. Specifically, intergal types, floating-point types and arithmetic types are specified in terms of other groups of types. I think it makes sense to follow the wording of the standard in code to make it trivial to verify. For example, arithmetic types is specified like this: Integral and floating-point types are collectively termed arithmetic types.. Written in code it's using arithmetic_types = concatenate_t<integral_types, floating_point_types>;.

53–57

The apply argument doesn't apply (I'm not sorry) anymore, but for_each is definitely a better name.

philnik edited the summary of this revision. (Show Details)Nov 17 2022, 11:20 PM
philnik updated this revision to Diff 476351.Nov 17 2022, 11:26 PM
philnik edited the summary of this revision. (Show Details)

Rebase

philnik updated this revision to Diff 476374.Nov 18 2022, 12:49 AM

Try to fix CI

ldionne accepted this revision.Nov 21 2022, 8:13 AM
ldionne added inline comments.
libcxx/test/support/test_iterators.h
1302–1314

For a second I thought T was the value_type.

libcxx/test/support/type_algorithms.h
22–26

Ok, agreed.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 21 2022, 11:35 AM
This revision was automatically updated to reflect the committed changes.
philnik marked an inline comment as done.

@philnik Sorry about the delay! I think I like the general direction. I do share some of the concerns re. complexity raised in the review -- there's definitely a tradeoff there -- but I also feel that verbosity and duplication are the biggest bane of our test suite. Especially with some of the simplifications made, this looks really nice and IMO doesn't add that much complexity compared to the amount of error-prone boilerplate that it removes. I haven't looked at this in detail, but the general idea LGTM.

var-const added inline comments.Dec 2 2022, 3:17 PM
libcxx/test/support/test_iterators.h
1307

Question: why does bidirectional_iterator_list include random access iterators? From the name, I would expect it to be _at most_ bidirectional.

philnik added inline comments.Dec 2 2022, 3:21 PM
libcxx/test/support/test_iterators.h
1307

I mean it the other way around, as-in "everything that is a bidirectional_iterator", like we need for the algorithm tests - bidirectional and everything better.

var-const added inline comments.Dec 2 2022, 3:24 PM
libcxx/test/support/test_iterators.h
1307

Ah, makes sense. Thanks for explaining.