Closed
Conversation
added 2 commits
May 14, 2024 08:44
Collaborator
Messing with the method signatures, might change the the size of the objects or the memory layout, regardless of private, public or protected. |
Author
|
Non virtual methods are stored the same way as free standing functions and do not change the object (even though not guaranteed by the standard). But given that you did a back port yourself I will close this one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a (manual) backport of #2495 to Iron as discussed in #2242.
As you might notice this PR does not go the way @jmachowinski suggested in #2242 (comment). (As I do not really see how these changes should be easily backported as they change quite a lot of the logic).
But I think the changes do not introduce an API/ABI break (in contrast to what @fujitatomoya wrote in #2242 (comment))
As stated in the developer guide
As this PR only changes the definition of private methods it is not necessary to recompile dependent code. I just tested this with our own code base (which uses ros2control with quite a lot of JTCs running at the same time) and it seemed to work fine.
( I only compiled the backport of rclcpp and rclcpp_action, enabled overrides for the workspace, made sure that I do not recompile our own code, and removed the system installation of rclcpp_action with sudo dpkg --force-all --remove ros-iron-rclcpp-action)
Given that it only changes private methods and that
EntityTypeis not exposed via any public methods I would argue that this backport also does not change the public API.Please note that I did not backport the adapted tests so far as I would like to hear some feedback first.
EDIT:
@fmauch perhaps it might be interesting for you to test this backport on your UR test setup.