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

Include pyAMReX.H & ODR Fix #149

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Jul 19, 2023

We provide a single helper include now for all pyAMReX compilation units (TUs) / cpp files.

This ensures that optional pybind11 includes, e.g., for stl, numpy, functions and operators, are consistently present in all TUs. This is required to avoid C++ one-definition-rule (ODR) violations, which are undefined behavior and thus could cause instabilities down the road.

@ax3l ax3l requested review from dpgrote and RTSandberg July 19, 2023 18:00
Copy link
Member

@RTSandberg RTSandberg left a comment

Choose a reason for hiding this comment

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

This looks good to me!

Copy link
Contributor

@dpgrote dpgrote left a comment

Choose a reason for hiding this comment

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

This looks good to me!

We provide a single helper include now for all pyAMReX compilation
units (TUs) / cpp files.

This ensures that optional pybind11 includes, e.g., for stl, numpy,
functions and operators, are consistently present in all TUs. This is
required to avoid C++ one-definition-rule (ODR) violations, which are
undefined behavior and thus could cause instabilities down the road.

void init_IntVect(py::module &m)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 141 lines.
@ax3l ax3l merged commit d5781f1 into AMReX-Codes:development Jul 19, 2023
15 checks passed
@ax3l ax3l deleted the topic-odr-include branch July 19, 2023 21:28
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

Successfully merging this pull request may close these issues.

3 participants