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: move static ParticleMap into header as inline for downstream use #1538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wdconinc
Copy link
Contributor

Briefly, what does this PR introduce?

Downstream plugins that use the ParticleSvc as an interface just include the header and are supposed to link against the library to get the static member. Having to load a shared library for just a static list is a bit annoying for a servicde interface definition.

This PR moves the static default list into an inline static definition (not a shared_ptr), and everything now lives in the header.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

@wdconinc wdconinc requested a review from a team July 27, 2024 18:21
@wdconinc wdconinc self-assigned this Jul 27, 2024
@wdconinc wdconinc requested review from rahmans1 and removed request for a team July 27, 2024 18:21
@veprbl veprbl added this to the 24.08.0 milestone Jul 28, 2024
Copy link
Contributor

@rahmans1 rahmans1 left a comment

Choose a reason for hiding this comment

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

Compiles and runs without issue. Slightly different output compared to what I get from standard eicrecon. Does this matter? @wdconinc

Screenshot 2024-07-29 124711b
Screenshot 2024-07-29 124649a

@wdconinc
Copy link
Contributor Author

Capybara claims no differences. This shouldn't introduce any differences at all, so I'm surprised it's showing differences for you.

@wdconinc
Copy link
Contributor Author

wdconinc commented Aug 7, 2024

@rahmans1 Can you explain why you see differences here? Is your 'standard' eicrecon the one just before this PR/commit, or could it include other merges? Based on capybara, I don't see any diffs.

@veprbl veprbl modified the milestones: 24.08.0, 24.09.0 Aug 17, 2024
@wdconinc wdconinc requested review from a team and rahmans1 and removed request for a team August 24, 2024 05:46
Copy link

sonarcloud bot commented Aug 24, 2024

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

Successfully merging this pull request may close these issues.

3 participants