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

[foxy] gazebo_ros: Add inline keyword to template definitions (#1367) #1389

Open
wants to merge 1 commit into
base: foxy
Choose a base branch
from

Conversation

jacobperron
Copy link
Collaborator

Port #1367 to Foxy.

* 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]>
Copy link

@adityapande-1995 adityapande-1995 left a 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).

@jacobperron
Copy link
Collaborator Author

That's a good question. I don't know and I'm not sure how to check. @j-rivero thoughts?

Copy link

@adityapande-1995 adityapande-1995 left a 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.

@jacobperron
Copy link
Collaborator Author

jacobperron commented May 11, 2022

I ran abi-compliance-checker locally on one of the consumers of the headers (libgazebo_ros_init.so) and it passed. Though this doesn't surprise me since it is just a consumer of the affected headers. I'm not sure how to properly confirm if this breaks ABI.

@jacobperron
Copy link
Collaborator Author

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.

@j-rivero
Copy link
Contributor

j-rivero commented Oct 5, 2022

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. nm -D --demangle on the libraries (consuming the templated functions) generated should shown if the functions are being translated to binary symbols or not, if I'm not wrong.

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.

4 participants