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 visiblity for linux: move explicit instanciations to the header file #565

Closed
wants to merge 3 commits into from

Conversation

j-rivero
Copy link
Contributor

@j-rivero j-rivero commented Oct 31, 2023

🦟 Bug fix

Summary

GCC was not happy with the use of a header only types in the explicit instantiations in order to generate the symbols in the library. Fixed by moving them to the header.

Part of the changes for gazebosim/gz-cmake#392

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Jose Luis Rivero <[email protected]>
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

Merging #565 (0653c5d) into main (beac11e) will not change coverage.
The diff coverage is n/a.

❗ Current head 0653c5d differs from pull request most recent head ec95d7a. Consider uploading reports for the commit ec95d7a to get more accurate results

@@           Coverage Diff           @@
##             main     #565   +/-   ##
=======================================
  Coverage   96.18%   96.18%           
=======================================
  Files         143      143           
  Lines        9826     9826           
=======================================
  Hits         9451     9451           
  Misses        375      375           
Files Coverage Δ
include/gz/math/MovingWindowFilter.hh 100.00% <ø> (ø)
src/MovingWindowFilter.cc 100.00% <ø> (ø)

@j-rivero
Copy link
Contributor Author

oh wonderful, there is now a different error in Mac and Windows.


/// The template explicit instantiations that does not use fundamental types
/// (like Vector*) needs to be placed in this header file.
template class GZ_MATH_VISIBLE MovingWindowFilter<int>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if putting explicit instantiations in a header is the right thing to do (see https://stackoverflow.com/questions/5864401/does-explicit-template-instantiation-go-in-cpp-or-header-file). If every user of this header instantiates these classes, we'd have ODR issues. Also, we'd lose the benefit of explicit instantiation since every translation unit will be instantiating the templates.

@j-rivero j-rivero marked this pull request as draft November 2, 2023 15:50
@j-rivero
Copy link
Contributor Author

j-rivero commented Nov 3, 2023

Current approach won't fix the issue and it triggers build warnings. I close this PR and open a new one.

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