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.

Sign up for GitHub

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

Jump to bottom

Modify access specifier to protected or public for the scope of processMessage() member function #984

Merged
merged 2 commits into from
May 17, 2023
Merged

Modify access specifier to protected or public for the scope of processMessage() member function #984

merged 2 commits into from
May 17, 2023

Conversation

hyunseok-yang
Copy link
Contributor

  • pose_display, pose_with_covariance_display and relative_humidity_display

@hyunseok-yang hyunseok-yang reopened this May 12, 2023
@hyunseok-yang hyunseok-yang mentioned this pull request May 12, 2023
Closed
@clalancette clalancette linked an issue May 15, 2023 that may be closed by this pull request
Suggest to changeAccess specifier in PoseWithCovarianceDisplay class #980
Closed
@hyunseok-yang
Modify access specifier to protected or public for the scope of proce…
8a1ff73 
…ssMessage() member function

- pose_display, pose_with_covariance_display
- marker_display, marker_array_display
- point_cloud_common
- relative_humidity_display, temperature_display, fluid_pressure_display, illuminance_display

Signed-off-by: (=YG=) Hyunseok Yang <hyunseok7.yang@lge.com>
@hyunseok-yang hyunseok-yang force-pushed the modify/rolling/displays/class/access-specifier branch from a9001a5 to 8a1ff73 Compare May 17, 2023 09:56
@hyunseok-yang
Copy link
Contributor Author

hyunseok-yang commented May 17, 2023 •
edited
Loading

@ahcorde @clalancette I rebased the branch on latest rolling. And fixed all related to access specifier for 'processMessage()' function.

@clalancette is it make sense? :)

@hyunseok-yang @clalancette
Update rviz_default_plugins/include/rviz_default_plugins/displays/flu…
366aa44 
…id_pressure/fluid_pressure_display.hpp

Co-authored-by: Chris Lalancette <clalancette@gmail.com>
hyunseok-yang
hyunseok-yang commented May 17, 2023
View reviewed changes
Copy link
Contributor Author

@hyunseok-yang hyunseok-yang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d'oh
thanks

...lugins/include/rviz_default_plugins/displays/relative_humidity/relative_humidity_display.hpp Show resolved Hide resolved
...lugins/include/rviz_default_plugins/displays/relative_humidity/relative_humidity_display.hpp Show resolved Hide resolved
clalancette
clalancette approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating. This looks good to me; I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette clalancette merged commit 9bb2495 into ros2:rolling May 17, 2023
1 check passed
@hyunseok-yang hyunseok-yang deleted the modify/rolling/displays/class/access-specifier branch May 18, 2023 11:22
@hyunseok-yang
Copy link
Contributor Author

@clalancette
Hi, Could you let me know when this changes can merged into humble branch?
I just wonder how the CI/CID process works :)

@clalancette
Copy link
Contributor

Hi, Could you let me know when this changes can merged into humble branch?

We have to consider further before backporting to humble. In particular, we have to be sure these changes don't change ABI. I don't think that they do, but I need to consider it further.

@hyunseok-yang
Copy link
Contributor Author

OK. Thanks for your comment.
Have a good day

@clalancette
Copy link
Contributor

Unfortunately, looking at this closer, it looks like this has the potential to change ABI. In particular, see https://community.kde.org/Policies/Binary_Compatibility_Examples#Change_the_access_rights . So we cannot backport this to humble as-is.

That said, there is a workaround mentioned in https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B (search for "Changing the access rights"). In particular, you could make a new protected method that called the existing private method. If you'd like to pursue that path, I'm happy to review the resulting PR.

@hyunseok-yang
Copy link
Contributor Author

Unfortunately, looking at this closer, it looks like this has the potential to change ABI. In particular, see https://community.kde.org/Policies/Binary_Compatibility_Examples#Change_the_access_rights . So we cannot backport this to humble as-is.

That said, there is a workaround mentioned in https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B (search for "Changing the access rights"). In particular, you could make a new protected method that called the existing private method. If you'd like to pursue that path, I'm happy to review the resulting PR.

Thanks for your review.
Understood in terms of 'changing the access rights'.

I'd like to make a new protected method which calls existing private function. I'm not sure this path makes consistency with other class.
And then, most important things is processMessage() function is called by somewhere else as a binding function. So it's not easy to implement what I expected.

I think workaround solution for just humble is not a way of good design.

Is there any other option to backport into humble? :(

@clalancette
Copy link
Contributor

Is there any other option to backport into humble? :(

Unfortunately no. We have to keep ABI in humble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde ahcorde is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest to changeAccess specifier in PoseWithCovarianceDisplay class
2 participants
@hyunseok-yang @clalancette