-
Notifications
You must be signed in to change notification settings - Fork 504
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
Replaced C-Style Cast with C++ Style Cast. #935
Conversation
Hey !! I just changed the C style casts to C++ style. Please review and see if it works fine :) |
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.
I found a few minor things. Thank you for this change! 🚀
moveit_core/robot_trajectory/include/moveit/robot_trajectory/robot_trajectory.h
Outdated
Show resolved
Hide resolved
moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…monitor.cpp Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
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.
I'm sorry to be so nit-picky. Thank you for helping with this. I'm just trying to avoid accidentally changing the existing behavior by moving what is cast to include more than just what would have been cast in with the c-style cast.
moveit_planners/ompl/ompl_interface/src/parameterization/model_based_state_space.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/ompl/ompl_interface/src/parameterization/model_based_state_space.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/ompl/ompl_interface/src/parameterization/model_based_state_space.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/sbpl/core/sbpl_interface/include/sbpl_interface/environment_chain3d_types.h
Outdated
Show resolved
Hide resolved
moveit_ros/visualization/rviz_plugin_render_tools/src/octomap_render.cpp
Outdated
Show resolved
Hide resolved
moveit_ros/visualization/rviz_plugin_render_tools/src/octomap_render.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…e/environment_chain3d_types.h Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…e/environment_chain3d_types.h Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…3d.cpp Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…age_octomap_updater.cpp Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…render.cpp Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
…render.cpp Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
Co-authored-by: Tyler Weaver <tylerjw@gmail.com>
if (x_index >= (int)cache_size_x_ || x_index < 0) | ||
if (x_index >= static_cast<int>(cache_size_x_ || x_index < 0)) | ||
{ | ||
ROS_DEBUG_NAMED("kinematics_cache", "X position %f,%d lies outside grid: %d %d", pose.position.x, x_index, 0, | ||
cache_size_x_); | ||
return false; | ||
} | ||
if (y_index >= (int)cache_size_y_ || y_index < 0) | ||
if (y_index >= static_cast<int>(cache_size_y_ || y_index < 0)) | ||
{ | ||
ROS_DEBUG_NAMED("kinematics_cache", "Y position %f,%d lies outside grid: %d %d", pose.position.y, y_index, 0, | ||
cache_size_y_); | ||
return false; | ||
} | ||
if (z_index >= (int)cache_size_z_ || z_index < 0) | ||
if (z_index >= static_cast<int>(cache_size_z_ || z_index < 0)) |
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.
Here are a few more where the parentheses change what is being cast.
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.
On to it. In a sec.
ROS_DEBUG_NAMED("kinematics_cache", "Read %d total points from file: %s", (int)num_solutions_vector_.size(), | ||
ROS_DEBUG_NAMED("kinematics_cache", "Read %d total points from file: %s", static_cast<int>num_solutions_vector_.size(), | ||
filename.c_str()); | ||
return true; | ||
} | ||
|
||
bool KinematicsCache::writeToFile(const std::string& filename) | ||
{ | ||
ROS_DEBUG_NAMED("kinematics_cache", "Writing %d total points to file: %s", (int)num_solutions_vector_.size(), | ||
ROS_DEBUG_NAMED("kinematics_cache", "Writing %d total points to file: %s", static_cast<int>num_solutions_vector_.size(), |
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.
These are missing the parentheses.
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.
Done.
Did the changes where it wasn't really a cast 😵💫. |
Only a couple more that I found and then this needs clang-tidy-10 ran on it to fix the formatting. We typically use a tool called
Then to run pre-commit to fix the formatting (do this in your checkout of the repo):
Then add the changes and push it. That will probably adjust a couple of small things. |
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.
This is ready to go after you run pre-commit and fix the formatting so the formatting test will pass. Thank you for this improvement. 😄
Codecov Report
@@ Coverage Diff @@
## main #935 +/- ##
==========================================
+ Coverage 57.91% 57.91% +0.01%
==========================================
Files 310 310
Lines 26067 26069 +2
==========================================
+ Hits 15093 15095 +2
Misses 10974 10974
Continue to review full report at Codecov.
|
I'm on a Mac and am facing issue installing clang-format 10. I ran pre-commit run --all and that's working fine. The only thing it says is command format 10 not found and I tried installing it using homebrew but no luck. What should I do ? |
For this PR, I can run it for you and update this PR. I don't know how to install clang-format-10 on a mac. The best idea I have is that you could maybe do that inside docker. You'd have to create a docker image and mount your directory and then you could use the apt command it installs it and then run it. |
|
Description
Please explain the changes you made, including a reference to the related issue if applicable
Checklist