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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

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



================
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*>>>();
+  });
----------------
Mordante wrote:
> 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.
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.


================
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
----------------
ldionne wrote:
> ~~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.
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>;`.


================
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);
----------------
ldionne wrote:
> How about this? I think it mirrors more closely what it does. It also removes the possibility for confusion between `apply` and `apply_all`.
The `apply` argument doesn't apply (I'm not sorry) anymore, but `for_each` is definitely a better name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137476