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

Fix wrong use of C msg headers #1844

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Jan 6, 2023

You inconsistently use C++ and C msg headers! This PR fixes all occurrences of wrong C header usage.

I'm really wondering why you didn't run into issues due to this before. When including moveit/utils/moveit_error_code.h, I'm getting these errors:

/opt/ros/rolling/include/moveit/utils/moveit_error_code.h:48:1: error: expected class-name before ‘{’ token
   48 | {
      | ^
/opt/ros/rolling/include/moveit/utils/moveit_error_code.h:54:43: error: ‘MoveItErrorCodes’ in namespace ‘moveit_msgs::msg’ does not name a type
   54 |   MoveItErrorCode(const moveit_msgs::msg::MoveItErrorCodes& code)

That's not surprising as the C++ and C types are fundamentally different, e.g.
moveit_msgs::msg::MoveItErrorCodes vs. moveit_msgs__msg__MoveItErrorCodes.

I guess you include the corresponding .hpp header somewhere else then...

@rhaschke rhaschke changed the title Cleanup msg includes: Use C++ instead of C header Fix wrong use of C msg headers Jan 6, 2023
@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Base: 50.43% // Head: 50.42% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (ad302b9) compared to base (8d1dace).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
- Coverage   50.43%   50.42%   -0.01%     
==========================================
  Files         374      374              
  Lines       31335    31335              
==========================================
- Hits        15802    15797       -5     
- Misses      15533    15538       +5     
Impacted Files Coverage Δ
.../distance_field/src/propagation_distance_field.cpp 93.84% <ø> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 65.97% <ø> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp 45.26% <0.00%> (-0.43%) ⬇️
...e/collision_detection_fcl/src/collision_common.cpp 73.76% <0.00%> (-0.22%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I found quite a few more, for example:

moveit_core/planning_scene/src/planning_scene.cpp:#include <octomap_msgs/conversions.h>

Found with:

grep -r ".h>" | grep "_msg"

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 6, 2023

You need to use this regex: /msgs/.*\.h(?!p) which I did.
The file you mention, @AndyZe, is not a generated message header.

@AndyZe
Copy link
Member

AndyZe commented Jan 6, 2023

You mean that messages from debians should be included differently than messages built along with MoveIt packages? Hmm, that's annoying. I'll defer to somebody else to review the PR, then.

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 6, 2023

I mean that only header files auto-generated via rosidl should be handled as suggested. The one you pointed out is a manually provided file: octomap_msgs/conversions.h

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Thank you!

@henningkayser henningkayser added the backport-humble Mergify label that triggers a PR backport to Humble label Jan 6, 2023
@henningkayser henningkayser merged commit 4765028 into moveit:main Jan 6, 2023
mergify bot pushed a commit that referenced this pull request Jan 6, 2023
(cherry picked from commit 4765028)

# Conflicts:
#	moveit_ros/moveit_servo/src/servo_calcs.cpp
#	moveit_ros/planning/moveit_cpp/include/moveit/moveit_cpp/planning_component.h
@rhaschke rhaschke deleted the fix-msg-includes branch January 8, 2023 15:29
rhaschke added a commit that referenced this pull request Jan 9, 2023
* Cleanup msg includes: Use C++ instead of C header
* Remove obsolete include: moveit_msgs/srv/execute_known_trajectory.hpp
ooeygui pushed a commit to ms-iot/moveit2 that referenced this pull request Jan 9, 2023
* Cleanup msg includes: Use C++ instead of C header
* Remove obsolete include: moveit_msgs/srv/execute_known_trajectory.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants