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.

Sign up for GitHub

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

Jump to bottom

Two suggestions to fix CommonADIOS1IOHandlerImpl #1101

Merged
merged 2 commits into from
Sep 15, 2021
Merged

Two suggestions to fix CommonADIOS1IOHandlerImpl #1101

merged 2 commits into from
Sep 15, 2021

Conversation

franzpoeschel
Copy link
Contributor

The current macro hack we have works, but is annoying:

  1. It's impossible to include other openPMD headers into CommonADIOS1IOHandlerImpl.cpp, since that file is itself included into the openPMD namespace. The first commit fixes the namespaces.
  2. IDEs usually don't understand what is going on and these macro things are not really compatible with the upcoming C++20 modules. The second commit suggests replacing things with a CRT pattern. Also has the advantage of having a separate class declaration for all things that are actually shared between the parallel and serial implementations.

@franzpoeschel franzpoeschel mentioned this pull request Sep 6, 2021
Merged
8 tasks
@ax3l ax3l self-requested a review September 11, 2021 01:45
@ax3l ax3l self-assigned this Sep 11, 2021
@franzpoeschel
Fix ADIOS1 namespaces
f5081e8 
With this fix, CommonADIOS1IOHandler.cpp is included outside of a
namespace. This makes it possible to properly include things inside
CommonADIOS1IOHandler.cpp without creating accidental double namespaces.
@franzpoeschel
Replace macro hack with CRT pattern
c684b14 
IDEs don't understand the macro thing and it keeps annoying me.
ax3l
ax3l approved these changes Sep 15, 2021
View reviewed changes
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you, that looks good!
I like replacing macros with CRT anytime :D

@ax3l ax3l merged commit e60c6a7 into openPMD:dev Sep 15, 2021
@ax3l ax3l mentioned this pull request Jan 13, 2022
Merged
@ax3l
Copy link
Member

ax3l commented Jan 13, 2022

Just saw that there is a regression in this PR when building an all-serial ADIOS1 build (via #1167).

ax3l added a commit that referenced this pull request Jan 14, 2022
@ax3l
ADIOS1: Fix Serial Builds, CI: Clang 10->12 (#1167)
3d66be9 
Update the Clang 10 build to Clang 12 in CI: commit 1 & 2.

Commit 3: ADIOS1 serial bugfixes: this is a regression of #1101 since around `CommonADIOS1IOHandler.cpp`
@ax3l ax3l mentioned this pull request Feb 8, 2022
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ax3l ax3l ax3l approved these changes

Assignees

@ax3l ax3l

Labels
backend: ADIOS1 internal refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@franzpoeschel @ax3l