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

[maint] cleanup includes #2351

Merged
merged 4 commits into from
Oct 6, 2020
Merged

[maint] cleanup includes #2351

merged 4 commits into from
Oct 6, 2020

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Oct 5, 2020

This PR uses forward declarations in MSA, wherever possible and moves most includes into .cpp files.
Additionally, this reverts 7b86d4d as we silent external warnings via system includes now.
Note, that this builds on top of #2349!

@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #2351 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2351   +/-   ##
=======================================
  Coverage   56.63%   56.63%           
=======================================
  Files         281      281           
  Lines       25457    25457           
=======================================
  Hits        14415    14415           
  Misses      11042    11042           
Impacted Files Coverage Δ
...on_distance_field/collision_distance_field_types.h 50.00% <ø> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 61.76% <0.00%> (ø)
...clude/moveit/occupancy_map_monitor/occupancy_map.h 28.58% <ø> (ø)
...warehouse/warehouse/src/moveit_message_storage.cpp 0.00% <ø> (ø)
...nning_scene_monitor/src/planning_scene_monitor.cpp 68.32% <0.00%> (-0.14%) ⬇️
...ipulation/pick_place/src/manipulation_pipeline.cpp 75.00% <0.00%> (+0.97%) ⬆️

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 8889901...302b0d7. Read the comment docs.

@rhaschke rhaschke force-pushed the cleanup-includes branch 2 times, most recently from b2b4ade to 302b0d7 Compare October 6, 2020 07:45
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.

I tested these and they don't seem to have any negative effects. Cleaning up the code is always nice. Thank you.

vspacer->setSizePolicy(QSizePolicy::Preferred, QSizePolicy::Expanding);
layout->addWidget(vspacer);
// Vertical Spacer
layout->addItem(new QSpacerItem(20, 20, QSizePolicy::Minimum, QSizePolicy::Expanding));
Copy link
Member

Choose a reason for hiding this comment

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

I know this is probably a normal QT style but all these new statements make me think it'd be easy to leak memory. I manually read through this function though and it doesn't look like we are here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Qt has its own memory management, releasing all children together with a parent object.
Adding items here to the layout adds the spacer as a child to the layout.

@rhaschke
Copy link
Contributor Author

rhaschke commented Oct 6, 2020

I will handle the conflict tomorrow.

@rhaschke rhaschke merged commit 7bad2f8 into moveit:master Oct 6, 2020
@rhaschke rhaschke deleted the cleanup-includes branch October 6, 2020 23:24
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
* *Changed boost namespace to std
*Need to compare function implementations
*Find an equivalent implementation for variate_generator

* calling Distribution(Engine) directly

* cleanup

* Seed mersenne twister variable with random device

* Updated with rsl random library
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.

2 participants