[libcxx-commits] [PATCH] D137476: [libc++] Add utilites for instantiating functions with multiple types

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D137476#3913144 <https://reviews.llvm.org/D137476#3913144>, @huixie90 wrote:

> In D137476#3910815 <https://reviews.llvm.org/D137476#3910815>, @EricWF wrote:
>
>> 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?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137476