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

Add the edm4hep includes to allow compilation with clang #166

Merged
merged 3 commits into from
Jan 27, 2024
Merged

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

  • Add the edm4hep includes to allow compilation with clang, which currently doesn't compile due to incomplete classes

ENDRELEASENOTES

Possible related discussion about different behavior for gcc and clang:
https://stackoverflow.com/questions/22770318/incomplete-types-in-template-code

With this change I can compile both on gcc and clang. Previously all the classes had to be specified anyway so having the includes is not that much of a change.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

I suppose it works for the LCIO types because they are used as pointers? Anyhow, no objections from my side here. We can then presumably remove the includes from the implementation file, right?

@jmcarcell
Copy link
Member Author

Yeah I think it's because of the pointers. I removed some includes, I'm not sure if I got them all

@jmcarcell jmcarcell merged commit 79d3dd4 into main Jan 27, 2024
9 of 13 checks passed
@jmcarcell jmcarcell deleted the clang branch January 27, 2024 17:49
jmcarcell added a commit that referenced this pull request Jan 29, 2024
jmcarcell added a commit that referenced this pull request Feb 5, 2024
…p build workflow; previous commit: Add the edm4hep includes to allow compilation with clang (#166)
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.

2 participants