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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
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>(); });
+
----------------
philnik wrote:
> Mordante wrote:
> > For a unit test this feels too clever for me.
> What exactly do you mean? Concatenating two cartesian products, the `caretesian_product_t` itself, or maybe something completely different?
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.




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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137476