Proposed cudf::type_dispatcher refactor per @bdice#9
Conversation
| stream); | ||
| } | ||
|
|
||
| template void fillOffsetsForDecimalSumState<int32_t>( |
There was a problem hiding this comment.
Are these explicit instantiations necessary?
| cudf::size_type numRows; | ||
| rmm::cuda_stream_view stream; | ||
|
|
||
| template <typename OffsetT> |
There was a problem hiding this comment.
Use SFINAE or a requires clause. We only want to compile the valid paths here, not all possible type dispatches. (Currently the invalid type dispatches are compiled to a function that does nothing, but we want to eliminate them with SFINAE/requires if possible.) That way you can have a non-void return type where it makes sense.
73a340e to
270f9db
Compare
d3f98fa to
1273f63
Compare
| stream); | ||
| } | ||
| template <> | ||
| void fillOffsetsForDecimalSumState::operator()<int32_t, 0>( |
There was a problem hiding this comment.
Why do we have duplicated explicit template instances? The body of the int32 and int64 are identical. This should be a template parameter.
| void operator()( | ||
| cudf::mutable_column_view offsetsView, | ||
| cudf::size_type numRows, | ||
| rmm::cuda_stream_view stream) const {} |
There was a problem hiding this comment.
Cursor says the stub "all other types" instantations are required. Not sure if your SFINAE/requires comment suggested some other way where they wouldn't be. Please advise.
2 participants
No description provided.