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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

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.



================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:111
+                                    meta::proxy_cpp20_input_iterator_list<int*>>>{},
+      []<class In, class Out>() { test_sentinels<In, Out>(); });
+
----------------
For a unit test this feels too clever for me.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:114
+  meta::apply_all(meta::cpp20_input_iterator_list<int*>{}, []<class Out> {
+    test_iterators<cpp20_input_iterator<int*>, Out, sentinel_wrapper<cpp20_input_iterator<int*>>>();
+  });
----------------
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.


================
Comment at: libcxx/test/std/language.support/support.limits/limits/is_specialized.pass.cpp:47
 {
-    test<bool>();
-    test<char>();
-    test<wchar_t>();
-#if TEST_STD_VER > 17 && defined(__cpp_char8_t)
-    test<char8_t>();
-#endif
-    test<char16_t>();
-    test<char32_t>();
-    test<signed char>();
-    test<unsigned char>();
-    test<signed short>();
-    test<unsigned short>();
-    test<signed int>();
-    test<unsigned int>();
-    test<signed long>();
-    test<unsigned long>();
-    test<signed long long>();
-    test<unsigned long long>();
-#ifndef TEST_HAS_NO_INT128
-    test<__int128_t>();
-    test<__uint128_t>();
-#endif
-    test<float>();
-    test<double>();
-    test<long double>();
-    static_assert(!std::numeric_limits<std::complex<double> >::is_specialized,
-                 "!std::numeric_limits<std::complex<double> >::is_specialized");
+  meta::apply_all(meta::arithmetic_types(), Test());
+
----------------
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.)


================
Comment at: libcxx/test/std/language.support/support.limits/limits/is_specialized.pass.cpp:48
-    test<char>();
-    test<wchar_t>();
-#if TEST_STD_VER > 17 && defined(__cpp_char8_t)
----------------
This actually a bug, it should not be available when `TEST_HAS_NO_WIDE_CHARACTERS` is defined.


================
Comment at: libcxx/test/std/strings/basic.string/string.access/at.pass.cpp:79
+TEST_CONSTEXPR_CXX20 bool test() {
+  meta::apply_all(meta::character_types(), TestCaller());
 
----------------
I consider this an improvement too, here we improve the test coverage.


================
Comment at: libcxx/test/support/type_algorithms.h:20
+
+// concatenates
+// - T                   -> type_list<T>
----------------
I like the amount of documentation.


================
Comment at: libcxx/test/support/type_algorithms.h:134
+using signed_integer_types =
+    type_list<short,
+              int,
----------------
`signed char` is a a signed integer type too.


================
Comment at: libcxx/test/support/type_algorithms.h:151
+
+using unsigned_integer_types = wrap_list_t<make_unsigned_t, signed_integer_types>;
+
----------------
This too look a bit clever. Here it's quite obvious, but in a different use-case it might be less obvious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137476