Skip to content

Removing some boost usage#1331

Merged
tylerjw merged 29 commits intomoveit:mainfrom
henrygerardmoore:remove_boost
Jun 15, 2022
Merged

Removing some boost usage#1331
tylerjw merged 29 commits intomoveit:mainfrom
henrygerardmoore:remove_boost

Conversation

@henrygerardmoore
Copy link
Copy Markdown
Contributor

@henrygerardmoore henrygerardmoore commented Jun 9, 2022 •
edited
Loading

Description

Taking over from #1118, this is part 1 of the work that will be done. More boost usage can be (somewhat) easily removed, but the PR is already quite large so that will take place in another PR. The changes included in this PR are

  • boost::array -> std::array
  • boost::function -> std::function
  • boost::mutex and similar objects (boost::shared_lock etc) -> std::mutex
  • boost::starts_with(name, object.first) -> name.rfind(object.first, 0) == 0
  • boost::math::tools::epsilon<double>() -> std::numeric_limits<double>::min()
  • boost::filesystem -> std::filesystem
  • boost::thread -> std::thread
  • Adding now-necessary includes
  • Changing some calls that differ between boost and the std equivalent

In a future PR, the following will be done:

  • replace boost::regex
  • replace boost::noncopyable
  • replace boost::lexical_cast
  • replace boost::optional
  • replace boost::thread_group

When MoveIt is updated to C++20, any use of boost::math::constants can be replaced with std::numbers constants

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

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 9, 2022 •
edited
Loading

Codecov Report

Merging #1331 (46670a2) into main (212af0f) will increase coverage by 0.02%.
The diff coverage is 29.47%.

@@            Coverage Diff             @@
##             main    #1331      +/-   ##
==========================================
+ Coverage   61.57%   61.58%   +0.02%     
==========================================
  Files         274      274              
  Lines       24965    24965              
==========================================
+ Hits        15369    15372       +3     
+ Misses       9596     9593       -3
Impacted Files Coverage Δ
...lude/moveit/collision_detection/collision_common.h 83.88% <ø> (ø)
...lude/moveit/collision_detection/collision_matrix.h 100.00% <ø> (ø)
...include/moveit/collision_detection/occupancy_map.h 0.00% <ø> (ø)
...tection/include/moveit/collision_detection/world.h 100.00% <ø> (ø)
..._core/collision_detection/src/collision_matrix.cpp 37.15% <ø> (ø)
...e/collision_detection_fcl/src/collision_common.cpp 73.76% <ø> (ø)
...sion_distance_field/collision_env_distance_field.h 4.77% <ø> (ø)
...it/collision_distance_field/collision_env_hybrid.h 0.00% <ø> (ø)
...e/include/moveit/kinematics_base/kinematics_base.h 75.76% <ø> (ø)
...ene/include/moveit/planning_scene/planning_scene.h 43.55% <ø> (ø)
... and 37 more

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 c491ac1...46670a2. Read the comment docs.

@henrygerardmoore henrygerardmoore changed the title Removing boost Removing some boost usage Jun 10, 2022
@Abishalini Abishalini mentioned this pull request Jun 10, 2022
Closed
6 tasks
@henrygerardmoore henrygerardmoore marked this pull request as ready for review June 10, 2022 21:05
@mergify
Copy link
Copy Markdown

mergify bot commented Jun 13, 2022

This pull request is in conflict. Could you fix it @henrygerardmoore?

henningkayser
henningkayser reviewed Jun 13, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for continuing on this effort. It's a lot of changes, but I assume it's fine squash-merging this since everything is pretty straight-forward. @Abishalini should be mentioned as co-author as she has prepared a sizeable portion of these changes (this can be added with the squash-merge commit).

@henrygerardmoore Are there any packages that don't depend on boost anymore? This should be reflected in the CMakeLists.txt fiels, possibly also in the package.xml dependencies. Could you verify this?

@mergify
Copy link
Copy Markdown

mergify bot commented Jun 14, 2022

This pull request is in conflict. Could you fix it @henrygerardmoore?

@henrygerardmoore
Copy link
Copy Markdown
Contributor Author

henrygerardmoore commented Jun 14, 2022

Removed boost from as many CMakeLists as I think is possible, and also made sure it wasn't unnecessarily in any package.xml files. I believe the PR should be good to go now.

Abishalini
Abishalini reviewed Jun 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Abishalini Abishalini left a comment

Choose a reason for hiding this comment

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

Found some headers that can be removed

Abishalini
Abishalini approved these changes Jun 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Abishalini Abishalini left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for this

tylerjw
tylerjw approved these changes Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thank you for taking this on and getting it over the finish line!

@tylerjw tylerjw merged commit 5ce32f9 into moveit:main Jun 15, 2022
@vatanaksoytezer vatanaksoytezer mentioned this pull request Jun 16, 2022
Closed
This was referenced Jun 16, 2022
Merged
Closed
@henrygerardmoore henrygerardmoore deleted the remove_boost branch June 17, 2022 16:37
mikeferguson added a commit to mikeferguson/moveit2 that referenced this pull request Jun 21, 2022
@mikeferguson
fix regression from moveit#1331
dc0d45b 
The noted PR swapped boost::thread to std::thread - however
std::thread will cause the program to crash with a note that
TERMINATE CALLED WITHOUT ACTIVE EXCEPTION if the thread object
goes out of scope while not joinable. Detaching the thread
restores the original workflow prior to the PR and allows
users to send more than one trajectory via the motion planning
plugin in RVIZ (otherwise, it crashes on the first trajectory).
mikeferguson added a commit that referenced this pull request Jun 21, 2022
@mikeferguson
fix regression from #1331 (#1384)
f4fc946 
The noted PR swapped boost::thread to std::thread - however
std::thread will cause the program to crash with a note that
TERMINATE CALLED WITHOUT ACTIVE EXCEPTION if the thread object
goes out of scope while not joinable. Detaching the thread
restores the original workflow prior to the PR and allows
users to send more than one trajectory via the motion planning
plugin in RVIZ (otherwise, it crashes on the first trajectory).
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
@henrygerardmoore @peterdavidfagan
Removing some boost usage (moveit#1331)
5cb2d25 
Co-authored-by: Vatan Aksoy Tezer <vatan@picknik.ai>
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
@mikeferguson @peterdavidfagan
fix regression from moveit#1331 (moveit#1384)
d85025c 
The noted PR swapped boost::thread to std::thread - however
std::thread will cause the program to crash with a note that
TERMINATE CALLED WITHOUT ACTIVE EXCEPTION if the thread object
goes out of scope while not joinable. Detaching the thread
restores the original workflow prior to the PR and allows
users to send more than one trajectory via the motion planning
plugin in RVIZ (otherwise, it crashes on the first trajectory).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tylerjw tylerjw tylerjw approved these changes

@henningkayser henningkayser henningkayser approved these changes

@Abishalini Abishalini Abishalini approved these changes

+1 more reviewer

@vatanaksoytezer vatanaksoytezer vatanaksoytezer approved these changes

Reviewers whose approvals may not affect merge requirements

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

@henrygerardmoore @tylerjw @henningkayser @vatanaksoytezer @Abishalini