[libcxx-commits] [PATCH] D154367: [libc++] mdspan - implement mdspan class

[Christian Trott via Phabricator via libcxx-commits]

crtrott added inline comments.


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/CustomTestLayouts.h:51
+template <class _Extents>
+class layout_wrapping_integral<_WrapArg>::mapping {
+  static constexpr typename _Extents::index_type _Wrap = static_cast<_Extents::index_type>(_WrapArg);
----------------
ldionne wrote:
> Would it make sense to use `layout_right::mapping` under the hood to implement this? It would reduce the complexity of this test class quite a bit, no?
I looked at it, because we wanted a layout which is runtime conditionally non-unique and not-strides, almost all the functions are different. So nothing to be saved really. 


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp:51
+
+  if constexpr (std::is_constructible_v<M, typename M::extents_type> && std::is_default_constructible_v<A>) {
+    if !consteval {
----------------
ldionne wrote:
> Whenever I see this kind of logic in tests, I feel like it would be too easy to make a mistake in the logic of the test and render the test useless. Instead, what would you think of passing whether you expect `H`, `M` and `A` to be constructible as bool template arguments to this function? Then this would be hardcoded at the point of call instead of figured out "automatically" by the test. This applies in a bunch of places.
OK done. 


================
Comment at: libcxx/test/std/containers/views/mdspan/mdspan/ctor.dh_integers.pass.cpp:51
+
+  if constexpr (std::is_constructible_v<M, typename M::extents_type> && std::is_default_constructible_v<A>) {
+    if !consteval {
----------------
crtrott wrote:
> ldionne wrote:
> > Whenever I see this kind of logic in tests, I feel like it would be too easy to make a mistake in the logic of the test and render the test useless. Instead, what would you think of passing whether you expect `H`, `M` and `A` to be constructible as bool template arguments to this function? Then this would be hardcoded at the point of call instead of figured out "automatically" by the test. This applies in a bunch of places.
> OK done. 
I did it for the ones where it seemed straight forward, and did not do it for conversion, where its kinda combinatorial what happens. That thing has more local checks to assure that we test what we believe we test.


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

https://reviews.llvm.org/D154367