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 missing inline attributes to avoid linker issues. #75

Merged
merged 3 commits into from Jan 27, 2024

Conversation

plietar
Copy link
Contributor

@plietar plietar commented Jan 15, 2024

The xoshiro<2>::do_jump specializations need some inline attribute, otherwise including dqrng from multiple compilation units causes linker errors due to symbols being defined multiple times.

While compiling against the latest pre-release, I hit some build errors which this should fix.

Firstly, the dqrng_types.h file was including Rcpp/Lightest, which should provide a slimmer interface to Rcpp. Unfortunately, the decision to include that file is "infectious" and impacts any compilation unit which includes dqrng. This is because including Rcpp/Lightest ends up defining Rcpp_hpp, and subsequent includes of Rcpp.h do nothing. My end applipcation was unable to use Rcpp's extended API because of this.

@rstub
Copy link
Member

rstub commented Jan 18, 2024

Thank you for your contribution. I can understand the second point. Sorry for the oversight! I am not so sure about the first point, though? Can you provide a minimal example that shows the issue? Wouldn't it be sufficient if you included Rccp.h before you include any dqrng related header? The latter point should probably be documented.

The `xoshiro<2>::do_jump` specializations need some `inline` attribute,
otherwise including dqrng from multiple compilation units causes linker
errors due to symbols being defined multiple times.
@plietar
Copy link
Contributor Author

plietar commented Jan 19, 2024

Wouldn't it be sufficient if you included Rccp.h before you include any dqrng related header?

Yes you're right. I don't know how I hadn't thought of this. Thanks!
I've changed the PR to only add the inline attributes.

The latter point should probably be documented

Let me know if/where you would want me to add something. Maybe we can raise this with Rcpp as well so they can provide guidance that can then be linked to from this repo.

@plietar plietar changed the title Fix a pair of build issues. Add missing inline attributes to avoid linker issues. Jan 19, 2024
@rstub
Copy link
Member

rstub commented Jan 20, 2024

I think I have found an even simpler solution: It is enough for dqrng to include Rcpp/XPtr.h instead of Rcpp/Lightest. Can you test if that works in your use-case independent of the include order?

@plietar
Copy link
Contributor Author

plietar commented Jan 22, 2024

Yes, that works for me as well, regardless of how I order my includes.

@rstub rstub merged commit 6b74223 into daqana:master Jan 27, 2024
@rstub
Copy link
Member

rstub commented Jan 27, 2024

Thanks a lot @plietar!

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.

None yet

2 participants