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

Providing default constructors for structs breaks use of designated initializers #329

Closed
ianabel opened this issue Aug 24, 2023 · 5 comments
Assignees

Comments

@ianabel
Copy link

ianabel commented Aug 24, 2023

Using Sundials in a C++20 program results in no clean way to instantiate custom SUNLinearSolvers (and other obejcts).

The usual

struct _generic_SUNLinearSolver_Ops LSOps =·
{
.gettype = SunLinSolWrapper::LSGetType,
.getid = SunLinSolWrapper::LSGetID,
.setatimes = nullptr, ...
};

Isn't permitted because the default constructor makes _generic_SUNLinearSolver_Ops a non-aggregate type.

From include/sundials/sundials_linearsolver.h:127

#ifdef __cplusplus
  _generic_SUNLinearSolver_Ops() = default;
#endif

Removing this default constructor results in a pure aggregate type and declaring the default constructor should not be necessary.

I know this is low priority, but flagging this as usage of C++20 will only increase.

@ianabel
Copy link
Author

ianabel commented Aug 24, 2023

This appears to have been added in the Ginko interface commit 86996b3 last october by @balos1 -- was it needed to get ginko working?

@drreynolds
Copy link
Collaborator

Thanks for bringing this up @ianabel. @balos1 is on travel this week, but hopefully he'll chime in soon.

@balos1
Copy link
Member

balos1 commented Sep 5, 2023

For reference, this issue occurs with C++20 because C++20 changed the definition of an aggregate so that it cannot have any constructor, while previously a default constructor was acceptable.

@ianabel
Copy link
Author

ianabel commented Sep 5, 2023

Right, I guess the question is -- was the default constructor needed for something specific.

By my reading the C++ standard defines the behaviour of constructing an aggregate to be exactly what the default constructor would have done (in this case, with simple types). So it should be fine to just remove, but if there's a particular reason it was there I'm happy to debug whether removing it is an issue.

@balos1
Copy link
Member

balos1 commented Sep 5, 2023

I cannot recall exactly why the default constructor was added. Removing it does not seem to break anything in my limited testing so far. If you are able to try it out as well, that would be great. Here is the branch where I removed it https://github.com/LLNL/sundials/tree/bugfix/cpp20-aggregate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants