-
Notifications
You must be signed in to change notification settings - Fork 67
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
Conversation
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #565 +/- ##
=======================================
Coverage 96.18% 96.18%
=======================================
Files 143 143
Lines 9826 9826
=======================================
Hits 9451 9451
Misses 375 375
|
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>; |
There was a problem hiding this comment.
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.
Signed-off-by: Jose Luis Rivero <[email protected]>
Current approach won't fix the issue and it triggers build warnings. I close this PR and open a new one. |
🦟 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
codecheck
passed (See contributing)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.