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

Ensure ABI stability with C++17/20 feature selection #10340

Closed
tamiko opened this issue May 24, 2020 · 6 comments · Fixed by #10346
Closed

Ensure ABI stability with C++17/20 feature selection #10340

tamiko opened this issue May 24, 2020 · 6 comments · Fixed by #10346
Assignees
Labels

Comments

@tamiko
Copy link
Member

tamiko commented May 24, 2020

We currently switch between boost and standard library variants of certain classes in our base/std_cxx17 and base/std_cxx20 dynamically based on whether a C++20 preprocessor macro is set.

This has one significant issue: If the library gets compiled with say default C++14 support we will select boost variants and compile the library with that. If a user now sets C++17/20 support in a user program the user code will use the standard library variants instead. This is an issue if user and library code must be ABI compatible.

A pragmatic solution would be to revert back to our old configuration logic and simply use the DEAL_II_HAVE_CXX17 macro instead and introduce a DEAL_II_HAVE_CXX20 for the corresponding C++20 features. The of this approach is the fact that the DEAL_II_HAVE_* are "configure time" constants and do not change with whatever a user enables in their own program.

@tamiko tamiko added the Bug label May 24, 2020
@tamiko tamiko added this to the Hackathon 2020 milestone May 24, 2020
@tamiko tamiko self-assigned this May 24, 2020
@tamiko
Copy link
Member Author

tamiko commented May 24, 2020

@masterleinad ping

@masterleinad
Copy link
Member

I am fine with adding the feature check macros to our configure time checks.

@bangerth
Copy link
Member

Ugh, yes. How did we not think of that back when...

@masterleinad
Copy link
Member

If you want me to, I can fix this later.

@tamiko
Copy link
Member Author

tamiko commented May 24, 2020

I am happy to make the change. Pull request incoming.

@drwells
Copy link
Member

drwells commented May 25, 2020

This seems like a reasonable candidate for 9.2.1. Should we start collecting such patches?

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

Successfully merging a pull request may close this issue.

4 participants