[Draft] showcase potential simplification of matrix op using concepts#1823
[Draft] showcase potential simplification of matrix op using concepts#1823tdavidcl wants to merge 1 commit into
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
There was a problem hiding this comment.
Code Review
This pull request refactors matrix operations to utilize C++20 concepts and simplifies template argument deduction across the library. Feedback suggests moving the newly introduced traits and concepts into the shammath namespace to avoid global scope pollution. Additionally, mdspan parameters should be passed by value, and constraints should be adjusted to allow for implicit type conversions in mat_set_vals and to restore type safety in mat_sub. An aliasing check for source and destination matrices is also recommended.
| template<class T> | ||
| struct is_mdspan : std::false_type {}; | ||
|
|
||
| template<class T, class Extents, class Layout, class Accessor> | ||
| struct is_mdspan<std::mdspan<T, Extents, Layout, Accessor>> : std::true_type {}; | ||
|
|
||
| template<class T> | ||
| inline constexpr bool is_mdspan_v = is_mdspan<std::remove_cvref_t<T>>::value; | ||
|
|
||
| template<class T> | ||
| concept Mdspan = is_mdspan_v<T>; | ||
|
|
||
| template<class T, std::size_t Rank> | ||
| concept MdspanRank = Mdspan<T> && (std::remove_cvref_t<T>::rank() == Rank); | ||
|
|
||
| template<class T> | ||
| using mdspan_value_t = typename std::remove_cvref_t<T>::value_type; | ||
|
|
||
| template<class A, class B> | ||
| concept mdspan_same_value = std::same_as<mdspan_value_t<A>, mdspan_value_t<B>>; |
| inline void mat_set_vals(MdspanRank<2> auto &input, Func &&func) | ||
| requires requires(Func f, int a, int b) { | ||
| { f(a, b) } -> std::same_as<mdspan_value_t<decltype(input)>>; | ||
| } |
There was a problem hiding this comment.
mdspan is a lightweight view and should be passed by value. Additionally, the requires clause uses std::same_as, which is overly restrictive as it prevents using functors that return types implicitly convertible to the matrix's value type (e.g., returning an int for a double matrix). Using std::convertible_to is more idiomatic and flexible.
| inline void mat_set_vals(MdspanRank<2> auto &input, Func &&func) | |
| requires requires(Func f, int a, int b) { | |
| { f(a, b) } -> std::same_as<mdspan_value_t<decltype(input)>>; | |
| } | |
| inline void mat_set_vals(MdspanRank<2> auto input, Func &&func) | |
| requires requires(Func f, int a, int b) { | |
| { f(a, b) } -> std::convertible_to<mdspan_value_t<decltype(input)>>; | |
| } |
Workflow reportworkflow report corresponding to commit d010790 Light CI is enabled. This will only run the basic tests and not the full tests. Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Doxygen diff with
|
No description provided.