fix view constructor constraints

[nmm0]

Note that this PR uses is_assignable on the mapping type for constraining the converting constructor and assignment for View., replacing is_assignable_data_type.

This is a breaking change; i.e. std::is_constructible_v<Kokkos::View<double[3][5], Kokkos::LayoutRight>, Kokkos::View<double[3][5], Kokkos::LayoutLeft>> was true before this change but is false now. However you couldn't actually construct the view because we would static assert in the body of the function.

Finally, I removed is_assignable_data_type. It was not used anywhere else and was a strict subset of is_assignable.

[masterleinad]

I am seeing

$ git grep -n "static_assert(Mapping::is_assignable" .
containers/src/Kokkos_DynRankView.hpp:1009:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_DynRankView.hpp:1020:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_DynRankView.hpp:1036:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_DynRankView.hpp:1047:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_DynamicView.hpp:483:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_OffsetView.hpp:822:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_OffsetView.hpp:837:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_OffsetView.hpp:857:    static_assert(Mapping::is_assignable,
containers/src/Kokkos_OffsetView.hpp:871:    static_assert(Mapping::is_assignable,
core/src/Kokkos_View.hpp:1353:    static_assert(Mapping::is_assignable,
core/src/Kokkos_View.hpp:1368:    static_assert(Mapping::is_assignable, "Incompatible View copy assignment");
core/src/Kokkos_View.hpp:1594:    static_assert(Mapping::is_assignable,
core/src/Kokkos_View.hpp:1609:    static_assert(Mapping::is_assignable,

Is there a good reason that you are not addressing all of them here?

[masterleinad]

What about

core/src/impl/Kokkos_ViewArray.hpp:426:    is_assignable_data_type =

?

[nmm0]

What about

core/src/impl/Kokkos_ViewArray.hpp:426:    is_assignable_data_type =

?

@masterleinad it seems to have been removed?

[nmm0]

Mapping::is_assignable

No I can do it

[masterleinad]

@masterleinad it seems to have been removed?

Ah, yes, needed to update my local develop branch.

[nmm0]

core/src/Kokkos_View.hpp:1594: static_assert(Mapping::is_assignable,
core/src/Kokkos_View.hpp:1609: static_assert(Mapping::is_assignable,

@masterleinad these are "semi-private" (i.e. not actually private but people shouldn't be using these constructors since the tracker type is impl. I'm not sure we need these to be constrained. I'll add the other ones though

[crtrott]

How public is is_assignable_data_type is there a reason to remove it other than you don't use it anymore?

[dalg24]

Please drop the & here and everywhere below.

[dalg24]

Considering how we always check assignable && constructible consider defining a is_constructible_and_assigable_v and a not_constructive_nor_assigable_v variable templates.

[dalg24]

What about test TestViewConversions

[dalg24]

How public is is_assignable_data_type is there a reason to remove it other than you don't use it anymore?

It is Impl::ViewMapping so not public but it breaks Trilinos.

packages/sacado/src/KokkosExp_View_Fad.hpp:1958:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad.hpp:2053:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:599:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1240:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1339:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1445:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1780:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad.hpp:831:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad.hpp:909:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad_Contiguous.hpp:696:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad_Contiguous.hpp:785:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1460:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1547:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1662:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1232:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1319:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1431:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1675:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1860:  enum { is_assignable_data_type = true };

[nmm0]

How public is is_assignable_data_type is there a reason to remove it other than you don't use it anymore?

It is Impl::ViewMapping so not public but it breaks Trilinos.

packages/sacado/src/KokkosExp_View_Fad.hpp:1958:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad.hpp:2053:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:599:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1240:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1339:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1445:  enum { is_assignable_data_type = true };
packages/sacado/src/KokkosExp_View_Fad_Contiguous.hpp:1780:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad.hpp:831:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad.hpp:909:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad_Contiguous.hpp:696:  enum { is_assignable_data_type = true };
packages/sacado/src/Kokkos_DynRankView_Fad_Contiguous.hpp:785:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1460:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1547:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp:1662:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1232:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1319:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1431:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1675:  enum { is_assignable_data_type = true };
packages/stokhos/src/sacado/kokkos/vector/KokkosExp_View_MP_Vector_Contiguous.hpp:1860:  enum { is_assignable_data_type = true };

@dalg24 does it break trilinos or those are just types that are trying to make conformant Kokkos mappings? If it's the later, those enums after this PR will just be useless, but not broken

[masterleinad]

I would assume it doesn't break Sacado/Stokhos but we should follow up with them.

[crtrott]

I have thought more about this: and I am not sure I want to "fix" this, this way. The basic issue is that is_constructible_v does not take into account mandates it only takes into account constraints.

Pre C++20 constraints provide unintelligible error messages where you have to parse SFINAE based enable ifs and what not to figure out what is going on. So we deliberately made some non-allowed stuff mandates, i.e. static asserts. Thus to enable checks if something is constructible from something else, Kokkos has the is_assignable traits/functions: i.e. Kokkos::is_assignable(dst, src);.

Now it is unfortunate that is_constructible_v doesn't take that into account. That is a reason why we avoid mandates in constructors inside of the standard.

But I am not convinced that the tradeoff of worse error messages is worth it to make is_constructible_v more workable in C++17. So here is what I propose:

We conditionally add require i.e. concepts if we compile with C++20 or newer. For now we leave the mandates in (doesn't hurt in C++20 mode). And then next year we remove the asserts. We can systematically look for those things in our codebase i.e. find static asserts in our constructors and check which of those could and should be constraints.