Skip to content

Combine serveral ConvertibleTo - #2052

Draft
yhmtsai wants to merge 1 commit into
developfrom
unified_convert
Draft

Combine serveral ConvertibleTo#2052
yhmtsai wants to merge 1 commit into
developfrom
unified_convert

Conversation

@yhmtsai

@yhmtsai yhmtsai commented Jul 21, 2026

Copy link
Copy Markdown
Member

We have ConvertibleTo and usually for each precision we support.
Previously, we use the next_precision and macro condition to support them properly.
Alternatively, we can merge them together and use template such that it inherit from all ConvertibleTo<Class<Precision, Rest...>>

Also, we can use mixin such that it use the template implementation automatically. (EnableConvertibleTo)
However, the mixin using the template member function does not implicit instantiate the function.

Some direction in my mind

  • only combine the inheritance: we will still have convert_to(next_precision) with macro
  • use mixin with template: need to have macro for instantiation
    • alternatively, use runtime dispatch in internal function, such that they are implicitly instantiated (I somehow like this way, but I also feel making some unused internal function is a bit weird)
  • use mixin with variant/runtime dispatch: similar to the above but merge runtime dispatch without template. However, it makes the result type to common type (or variant) and then cast back to the result type.
  • any idea?

@yhmtsai
yhmtsai requested review from MarcelKoch and pratikvn July 21, 2026 09:14
@yhmtsai yhmtsai self-assigned this Jul 21, 2026
@yhmtsai yhmtsai added the 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. label Jul 21, 2026
@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats labels Jul 21, 2026
Comment on lines +56 to +57
gko::detail::get_precision_list<ValueType,
gko::detail::precision_list>>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think the type could better be called conversion_types.

void convert_to(
Coo<next_precision<ValueType>, IndexType>* result) const override;
template <typename ResultValueType>
void convert_to_impl(Coo<ResultValueType, IndexType>* result) const;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have a *_impl function on the public interface. Either is should be renamed, or made private.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be in private although it will need all EnableConvertibleTo<*> to be friend of all class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants