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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.

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.

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.



================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:105
 
-template <class Out>
-constexpr void test_in_iterators() {
-  test_iterators<cpp20_input_iterator<int*>, Out, sentinel_wrapper<cpp20_input_iterator<int*>>>();
-  test_sentinels<forward_iterator<int*>, Out>();
-  test_sentinels<bidirectional_iterator<int*>, Out>();
-  test_sentinels<random_access_iterator<int*>, Out>();
-  test_sentinels<contiguous_iterator<int*>, Out>();
-}
-
-template <class Out>
-constexpr void test_proxy_in_iterators() {
-  test_iterators<ProxyIterator<cpp20_input_iterator<int*>>, Out, sentinel_wrapper<ProxyIterator<cpp20_input_iterator<int*>>>>();
-  test_iterators<ProxyIterator<forward_iterator<int*>>, Out>();
-  test_iterators<ProxyIterator<bidirectional_iterator<int*>>, Out>();
-  test_iterators<ProxyIterator<random_access_iterator<int*>>, Out>();
-  test_iterators<ProxyIterator<contiguous_iterator<int*>>, Out>();
-}
-
 constexpr bool test() {
+  meta::apply_all(
----------------
@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>>>();
    });
  });
```


================
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());
 
----------------
Mordante wrote:
> I consider this an improvement too, here we improve the test coverage.
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.


================
Comment at: libcxx/test/support/type_algorithms.h:16
+
+namespace meta {
+template <class... Types>
----------------
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.


================
Comment at: libcxx/test/support/type_algorithms.h:21-25
+// - T                   -> type_list<T>
+// - T, U                -> type_list<T, U>
+// - type_list<Ts...>    -> type_list<Ts...>
+// - type_list<Ts...>, T -> type_list<Ts..., T>
+// - N type_lists to one
----------------
~~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.


================
Comment at: libcxx/test/support/type_algorithms.h:50
+template <class... Types, class Functor>
+TEST_CONSTEXPR_CXX14 void apply(type_list<Types...>, Functor f);
+
----------------
I don't think that's needed anymore if we don't have `cartesian_product`.


================
Comment at: libcxx/test/support/type_algorithms.h:52-56
+// apply_all takes a type_list of type_lists and applies all the type_lists to f
+// If only a single type is required, the second type_list can be dropped.
+// i.e. type_list<int> and type_list<type_list<int>> are equivalent
+template <class... TypeLists, class Functor>
+TEST_CONSTEXPR_CXX14 void apply_all(type_list<TypeLists...>, Functor f);
----------------
How about this? I think it mirrors more closely what it does. It also removes the possibility for confusion between `apply` and `apply_all`.


================
Comment at: libcxx/test/support/type_algorithms.h:90
+template <template <class> class Wrapper, class... Types>
+struct wrap_list<Wrapper, type_list<Types...> > {
+  using type = type_list<Wrapper<Types>...>;
----------------
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.


================
Comment at: libcxx/test/support/type_algorithms.h:130
+#endif
+              char16_t,
+              char32_t>;
----------------
This seems to be >= C++11, and also `char32_t`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137476