-
Notifications
You must be signed in to change notification settings - Fork 290
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
Remove visibility from headers that are not installed #665
Conversation
Signed-off-by: Louise Poubel <[email protected]>
Codecov Report
@@ Coverage Diff @@
## ign-gazebo3 #665 +/- ##
===============================================
- Coverage 77.87% 77.82% -0.05%
===============================================
Files 212 210 -2
Lines 11719 11649 -70
===============================================
- Hits 9126 9066 -60
+ Misses 2593 2583 -10
Continue to review full report at Codecov.
|
ABI checker is happy, opening for review (CC @j-rivero ) |
See your comment here: #563 (comment)
Do you have the build number? What branch were you testing? Because I think |
aaah ok. I remember that there was a reason for it. Thanks! |
🦟 Bug fix
Summary
Backporting part of the changes from #501.
I believe it should be fine to change the visibility of classes that are not installed in a stable distribution, let's see what the ABI checker says.
The motivation is to make forward-porting changes easier. On #622 we had to make lots of changes for the code ported from Dome to Edifice to work on Windows. If we encourage proper usage of visibility flags from Citadel, new plugins should use correct boilerplate from the beginning.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge