Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GCC 7.5 issue with distributed::Matrix #1696

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

MarcelKoch
Copy link
Member

This PR uses the xstd::void_t again in is_matrix_type_builder. I think there is a bug in GCC 7.5 bug when std::void_t is used for is_matrix_type_builder. I wasn't able to reproduce this issue to actually find out if it's a bug or not, but since this is not an issue in newer gcc version, I don't think it's worth to put more effort into it.

@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label Oct 15, 2024
@MarcelKoch MarcelKoch added this to the Ginkgo 1.9.0 milestone Oct 15, 2024
@MarcelKoch MarcelKoch requested a review from a team October 15, 2024 08:02
@MarcelKoch MarcelKoch self-assigned this Oct 15, 2024
@ginkgo-bot ginkgo-bot added the mod:core This is related to the core module. label Oct 15, 2024
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

could you paste the error message in the pr?
It might be helpful for searching when we face the same issue in other places.

@MarcelKoch
Copy link
Member Author

I think this is the important part:

In file included from /home/marcel/projects/ginkgo/include/ginkgo/ginkgo.hpp:64:0,
                 from /home/marcel/projects/ginkgo/benchmark/solver/distributed/solver.cpp:10:
/home/marcel/projects/ginkgo/include/ginkgo/core/distributed/matrix.hpp: In instantiation of ‘struct gko::detail::is_matrix_type_builder<std::unique_ptr<gko::LinOp, std::default_delete<gko::LinOp> >, double, int, void>’:
/home/marcel/projects/ginkgo/include/ginkgo/core/distributed/matrix.hpp:481:73:   required by substitution of ‘template<class LocalMatrixType, class NonLocalMatrixType, class> static std::unique_ptr<gko::experimental::distributed::Matrix<double, int, long int>, std::default_delete<gko::experimental::distributed::Matrix<double, int, long int> > > gko::experimental::distributed::Matrix<double, int, long int>::create<LocalMatrixType, NonLocalMatrixType, <template-parameter-1-3> >(std::shared_ptr<const gko::Executor>, gko::experimental::mpi::communicator, LocalMatrixType, NonLocalMatrixType) [with LocalMatrixType = std::unique_ptr<gko::LinOp, std::default_delete<gko::LinOp> >; NonLocalMatrixType = std::unique_ptr<gko::LinOp, std::default_delete<gko::LinOp> >; <template-parameter-1-3> = <missing>]’
/home/marcel/projects/ginkgo/benchmark/utils/generator.hpp:245:49:   required from here
/home/marcel/projects/ginkgo/include/ginkgo/core/distributed/matrix.hpp:59:51: error: ‘class std::unique_ptr<gko::LinOp, std::default_delete<gko::LinOp> >’ has no member named ‘create’
         decltype(std::declval<Builder>().template create<ValueType, IndexType>(
                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/marcel/projects/ginkgo/include/ginkgo/core/distributed/matrix.hpp:59:51: error: ‘class std::unique_ptr<gko::LinOp, std::default_delete<gko::LinOp> >’ has no member named ‘create’

Somehow the missing create function is not triggering SFINAE in this context.

@yhmtsai yhmtsai requested a review from a team October 15, 2024 08:59
Copy link
Collaborator

@greole greole left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -27,8 +27,24 @@ namespace gko {
* @ingroup xstd
*/
namespace xstd {
namespace detail {


Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add some documentation on what the intended use is.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no use for users for this. On the developer side it's used directly below, so I don't think it requires documentation.

@MarcelKoch MarcelKoch added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Oct 15, 2024
This works around a (likely) GCC 7.5 bug when using std::void_t.
@MarcelKoch MarcelKoch added the 1:ST:no-changelog-entry Skip the wiki check for changelog update label Oct 15, 2024
Copy link

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.96%. Comparing base (f6eb48a) to head (ca3f152).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1696      +/-   ##
===========================================
- Coverage    90.24%   89.96%   -0.28%     
===========================================
  Files          763      763              
  Lines        62841    62877      +36     
===========================================
- Hits         56713    56570     -143     
- Misses        6128     6307     +179     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MarcelKoch MarcelKoch merged commit ca79f34 into develop Oct 16, 2024
12 of 14 checks passed
@MarcelKoch MarcelKoch deleted the fix-distmat-gcc7 branch October 16, 2024 06:40
MarcelKoch added a commit to MarcelKoch/ginkgo that referenced this pull request Dec 2, 2024
This merge uses the xstd::void_t again in is_matrix_type_builder. I think there is a bug in GCC 7.5 bug when std::void_t is used for is_matrix_type_builder, although I wasn't able to reproduce it.

Related PR: ginkgo-project#1696
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. mod:core This is related to the core module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants