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

update cast to modern style #418

Merged
merged 3 commits into from
Mar 28, 2023
Merged

update cast to modern style #418

merged 3 commits into from
Mar 28, 2023

Conversation

marioprats
Copy link
Contributor

A minor update of an old-style cast to suppress warnings downstream.

@marioprats
update cast to modern style
e430ffb 
Signed-off-by: Mario Prats <marioprats@gmail.com>
@fujitatomoya
Copy link
Collaborator

CI:

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

@iuhilnehc-ynos
Copy link
Collaborator

iuhilnehc-ynos commented Mar 24, 2023 •
edited
Loading

What if a C library uses rcutils to call this macro(RCUTILS_LOG_DEBUG_THROTTLE -> RCUTILS_LOG_CONDITION_THROTTLE_BEFORE)? Does it work?

to suppress warnings makes sense. What about checking the __cplusplus macro, something like(NOT TESTED),

#ifdef __cplusplus
  #define CAST_DURATION(x) (static_cast<rcutils_duration_value_t>(x))
#else
  #define CAST_DURATION(x) ((rcutils_duration_value_t)x)
#endif

#define RCUTILS_LOG_CONDITION_THROTTLE_BEFORE(get_time_point_value, duration) { \
static rcutils_duration_value_t __rcutils_logging_duration = RCUTILS_MS_TO_NS(CAST_DURATION(duration));

@fujitatomoya
Copy link
Collaborator

Ah, yeah i think it does not, for example rclc?

clalancette
clalancette requested changes Mar 24, 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.

We cannot take this. This is meant to be a C header, includable and usable by both C and C++. So the code is correct as-is.

@clalancette
Copy link
Contributor

@marioprats Can you give more details on what problems this is causing you?

@marioprats
Copy link
Contributor Author

@marioprats Can you give more details on what problems this is causing you?

I noticed this was triggering warnings in some packages downstream and thought I could fix it at the source. Didn't realize this is a C library.
Let me take another look, thanks!

@marioprats
Add CAST_DURATION() to support C++ style duration cast
47fc5df 
Signed-off-by: Mario Prats <marioprats@gmail.com>
@marioprats
Copy link
Contributor Author

marioprats commented Mar 25, 2023 •
edited
Loading

Here's one example of a warning while building moveit2:

--- stderr: moveit_visual_tools In file included from /home/marioprats/moveit2_ws/build/rclcpp/include/rclcpp/logging.hpp:24, from /home/marioprats/moveit2_ws/install/moveit_core/include/moveit_core/moveit/kinematics_base/kinematics_base.h:42, from /home/marioprats/moveit2_ws/install/moveit_core/include/moveit_core/moveit/robot_model/joint_model_group.h:42, from /home/marioprats/moveit2_ws/install/moveit_core/include/moveit_core/moveit/robot_model/robot_model.h:45, from /home/marioprats/moveit2_ws/install/moveit_core/include/moveit_core/moveit/robot_state/robot_state.h:40, from /home/marioprats/moveit2_ws/install/moveit_core/include/moveit_core/moveit/robot_state/conversions.h:39, from /home/marioprats/moveit2_ws/src/moveit_visual_tools/src/imarker_robot_state.cpp:34: /home/marioprats/moveit2_ws/src/moveit_visual_tools/src/imarker_robot_state.cpp: In function ‘bool {anonymous}::isIKStateValid(const planning_scene::PlanningScene*, bool, bool, const MoveItVisualToolsPtr&, moveit::core::RobotState*, const moveit::core::JointModelGroup*, const double*)’: /home/marioprats/moveit2_ws/src/moveit_visual_tools/src/imarker_robot_state.cpp:101:48: warning: use of old-style cast to ‘rcutils_duration_value_t’ {aka ‘long int’} [-Wold-style-cast] 101 | RCLCPP_WARN_THROTTLE(LOGGER, steady_clock, 2000, "Collision in IK CC callback");

The warning is triggered by the THROTTLE macros in rcutils with the C-style cast, when used from rclcpp (logging.hpp) in C++.
The solution proposed by Chen in this thread seems to work, so I updated the code to do that.
LMK if I'm still missing anything...

@marioprats marioprats requested review from clalancette and removed request for wjwwood and ahcorde March 25, 2023 06:11
clalancette
clalancette requested changes Mar 26, 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.

Besides the stylistic thing, I think this should be OK.

resource/logging_macros.h.em Outdated Show resolved Hide resolved
@iuhilnehc-ynos
Copy link
Collaborator

Could you add the prefix RCUTILS_ for CAST_DURATION?

@marioprats
add RCUTILS_ prefix to CAST_DURATION
16b6206 
Signed-off-by: Mario Prats <marioprats@gmail.com>
@marioprats
Copy link
Contributor Author

Could you add the prefix RCUTILS_ for CAST_DURATION?

Added the RCUTILS_ prefix

@clalancette
Copy link
Contributor

CI:

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

@clalancette clalancette merged commit 5bd1230 into ros2:rolling Mar 28, 2023
@marioprats marioprats deleted the new_style_cast branch March 28, 2023 18:44
@outrider-jhulas
Copy link

Could we add this change to humble?

@fujitatomoya
Copy link
Collaborator

@Mergifyio backport iron humble

@mergify Mergify
Copy link

mergify bot commented Apr 19, 2024 •
edited
Loading

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Apr 19, 2024
@marioprats @mergify
update cast to modern style (#418)
086d734 
* update cast to modern style

Signed-off-by: Mario Prats <marioprats@gmail.com>
(cherry picked from commit 5bd1230)
@mergify mergify bot mentioned this pull request Apr 19, 2024
Merged
ahcorde pushed a commit that referenced this pull request Apr 22, 2024
@mergify @marioprats
update cast to modern style (#418) (#462)
5572dd0 
* update cast to modern style

Signed-off-by: Mario Prats <marioprats@gmail.com>
(cherry picked from commit 5bd1230)

Co-authored-by: Mario Prats <marioprats@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@clalancette clalancette clalancette approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@marioprats @fujitatomoya @iuhilnehc-ynos @clalancette @outrider-jhulas