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

Use sdf FindElement API to avoid const_cast #2231

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Nov 7, 2023

🦟 Bug fix

Fixes unneeded const_cast calls.

Summary

Several systems use const_cast in order to call sdf::Element::GetElement with const ElementPtrs, but the FindElement API can be used instead.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Several systems use const_cast in order to call
sdf::Element::GetElement with const ElementPtrs,
but the FindElement API can be used instead.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters requested a review from mjcarroll as a code owner November 7, 2023 05:27
// Get params from SDF
sdf::ElementPtr sdfElem = ptr->GetElement("left_joint");
sdf::ElementConstPtr sdfElem = _sdf->FindElement("left_joint");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually a good use case for auto. We shouldn't really care what the type is and the compiler should enforce the const correctness.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I'll update this and similar types to auto

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a fever, and the prescription was more auto

7b6e966

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #2231 (7b2fd36) into main (9407311) will decrease coverage by 0.03%.
Report is 3 commits behind head on main.
The diff coverage is 90.90%.

❗ Current head 7b2fd36 differs from pull request most recent head 9cf0824. Consider uploading reports for the commit 9cf0824 to get more accurate results

@@            Coverage Diff             @@
##             main    #2231      +/-   ##
==========================================
- Coverage   65.90%   65.88%   -0.03%     
==========================================
  Files         323      323              
  Lines       30719    30713       -6     
==========================================
- Hits        20245    20234      -11     
- Misses      10474    10479       +5     
Files Coverage Δ
src/systems/diff_drive/DiffDrive.cc 83.75% <100.00%> (-0.06%) ⬇️
...stems/joint_state_publisher/JointStatePublisher.cc 84.37% <100.00%> (-0.13%) ⬇️
...ms/joint_traj_control/JointTrajectoryController.cc 79.31% <100.00%> (-0.06%) ⬇️
src/systems/log/LogRecord.cc 79.48% <100.00%> (-0.08%) ⬇️
src/systems/velocity_control/VelocityControl.cc 90.22% <100.00%> (-0.08%) ⬇️
src/systems/tracked_vehicle/TrackedVehicle.cc 65.86% <75.00%> (-0.11%) ⬇️

... and 3 files with indirect coverage changes

@iche033
Copy link
Contributor

iche033 commented Nov 8, 2023

looks good to me. Windows CI needs gazebosim/gz-common#550

@mjcarroll
Copy link
Contributor

@osrf-jenkins retest this please

@scpeters
Copy link
Member Author

CI issues are unrelated, merging

@scpeters scpeters merged commit efc3bed into main Nov 10, 2023
6 of 8 checks passed
@scpeters scpeters deleted the scpeters/rm_const_cast_sdf branch November 10, 2023 19:21
scpeters added a commit that referenced this pull request Nov 10, 2023
Several systems use const_cast in order to call
sdf::Element::GetElement with const ElementPtrs,
but the FindElement API can be used instead.

Signed-off-by: Steve Peters <[email protected]>
mjcarroll pushed a commit that referenced this pull request Nov 13, 2023
Several systems use const_cast in order to call
sdf::Element::GetElement with const ElementPtrs,
but the FindElement API can be used instead.

Signed-off-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants