-
Notifications
You must be signed in to change notification settings - Fork 776
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
[foxy] gazebo_ros: Add inline keyword to template definitions (#1367) #1389
base: foxy
Are you sure you want to change the base?
Conversation
* gazebo_ros: Add inline keyword to template definitions * Uncrustify Signed-off-by: Jacob Perron <[email protected]> Co-authored-by: Gerard Heshusius <[email protected]> Co-authored-by: Jacob Perron <[email protected]>
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.
Are we sure this does not break ABI ? From https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B , changing an existing function of any type to inline is not allowed:
You cannot :
For existing functions of any type:
[Unexport](https://community.kde.org/Policies/Binary_Compatibility_Examples#Unexport_a_function) it.
Remove it.
Remove the implementation of existing declared functions. The symbol comes from the implementation of the function, so this is effectively the function.
[Inline](https://community.kde.org/Policies/Binary_Compatibility_Examples#Inline_a_function) it (this includes moving a member function's body to the class definition, even without the inline keyword).
That's a good question. I don't know and I'm not sure how to check. @j-rivero thoughts? |
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.
Since this an important bugfix and we already have this merged in galactic (probably breaking ABI), I'm inclined to approve this but would like to hear @j-rivero 's and @chapulina's thoughts.
I ran |
Since this isn't released into Galactic, we can still decide to revert the change, though I'm inclined to keep it since it is a bugfix. |
Sorry for arriving late. I'm also under the impression that this is ABI breaking change, something that historically we have tried to avoid in the released versions of the repo although I've been without watching it for long. My C++ is a bit rusty but if I'm not wrong: the templates should generate the needed functions (and binary symbols) at compilation time for the different instantiations of the consumers, if we make them inline then the binary symbols for the function won't be generated. |
Port #1367 to Foxy.