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 syntax #939

Merged
merged 2 commits into from
Dec 24, 2021
Merged

Fix syntax #939

merged 2 commits into from
Dec 24, 2021

Conversation

sjahr
Copy link
Contributor

@sjahr sjahr commented Dec 23, 2021

Description

Replace a comma with a semicolon. I am a little bit confused, why did this build before and why doesn't CI complain about this?

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

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.

Wow.

Copy link
Contributor

@vatanaksoytezer vatanaksoytezer left a comment

Choose a reason for hiding this comment

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

Wow indeed, I should investigate why CI didn't catch this.

@sjahr
Copy link
Contributor Author

sjahr commented Dec 23, 2021

But this

hp_action_client_ = rclcpp_action::create_client<moveit_msgs::action::HybridPlanner>(node_, "run_hybrid_planning"),

builds in my local setup too, so maybe CI is fine.

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #939 (299d872) into main (ec31a54) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
+ Coverage   57.91%   57.91%   +0.01%     
==========================================
  Files         310      310              
  Lines       26069    26069              
==========================================
+ Hits        15095    15096       +1     
+ Misses      10974    10973       -1     
Impacted Files Coverage Δ
...nning_scene_monitor/src/planning_scene_monitor.cpp 53.31% <0.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec31a54...299d872. Read the comment docs.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

It builds fine because similarly, this is valid c++ syntax:

a = 2, b = 3;

The comma operator can be used to separate multiple assignment statements, there is no requirement that you have to put a semi-colon at the end of a line, only that you have to finish each statement with them.

SO post about comma operator: https://stackoverflow.com/questions/4352857/what-is-a-comma-separated-set-of-assignments

@sjahr
Copy link
Contributor Author

sjahr commented Dec 24, 2021

Ahh, thanks for making this clear, I totally forgot about this syntax option. Should we merge it anyways for uniformity and to avoid further confusion?

@AndyZe AndyZe merged commit 52ffbfd into moveit:main Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants