-
Notifications
You must be signed in to change notification settings - Fork 943
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
bind() them all (not) #3106
bind() them all (not) #3106
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3106 +/- ##
==========================================
- Coverage 61.62% 61.57% -0.05%
==========================================
Files 376 376
Lines 33275 33310 +35
==========================================
+ Hits 20504 20508 +4
- Misses 12771 12802 +31
Continue to review full report at Codecov.
|
269f01e
to
edac397
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all this cleanup work. I like all the changes.
However, in a few locations you also changed public API (changing pointer arguments to references to indicate that valid instances are expected.)
I'm fine with these changes as well, but I'm surprised to see them proposed from you. Don't you argue against public API changes usually?
I guess you already noticed that several tests are broken now.
{ | ||
// boost bind is not happy with overloading, so we add intermediate function objects | ||
|
||
bool callAdapter1(const PlanningRequestAdapter* adapter, const planning_interface::PlannerManagerPtr& planner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great cleanup!
@@ -47,21 +47,21 @@ | |||
const int TRIALS = 1000; | |||
const int THREADS = 2; | |||
|
|||
bool runCollisionDetection(unsigned int /*id*/, unsigned int trials, const planning_scene::PlanningScene* scene, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning up this kind of pointer usage!
I shortly looked into the first failing test and it is very strange: The ModelBasedStateSpaceFactoryPtr returned from |
the trailing 1 and 2 and bind() usage made this harder to parse than it has to be. Technically this breaks API (the methods are protected, not private). But if you are that far into the code base to inherit here and rely on the names of these simple methods, you probably agree that this improves readability.
04eea20
to
681bc2b
Compare
Thanks for the additional patches! I rebased for a clean history. Yes, I saw the failing tests already, but just finished the patch yesterday and wanted to push it already. I just started to look into it a bit further as well, but could not understand it right away.
Messy. I see the same issue with clang 13 (did not get to upgrade to 14 yet), so I'm not sure it's a compiler issue. We probably need to find a workaround whether or not we can pinpoint an external bug. Speaking about API: All the header-specific changes I made address |
Yes, probably it is a bug in libstdc++. At some point in the call hierarchy the returned shared_ptr reference is copied (which is wrong in first place) and then released.
Sorry, I was convinced to have seen a public declaration. Checking again, you are right, everything is private or protected. |
You are right, I didn't noticed this before. I suggested two approaches to fix the issue in v4hn#3 - both changing protected API of OMPL::PlanningContextManager, which is fine I think. Given your comment, I prefer the last one. |
Makes sense. I cherry-picked your proposed modification of the protected |
Sorry, I forgot to remove the unused |
Closing and reopening to trigger CI... |
I tried to reproduce the shared_ptr issue in a minimal example, but there it works: https://godbolt.org/z/T68jzMYW4. |
@v4hn, I think this is ready to merge after squash-merging the fixups. |
It made the code hard to understand in many places and was needed because lambdas did not exist when this was written. Times change.
to ensure people use lambdas. Look anywhere. That's what you should do.
... to improve readability
- Calling the factory_selector lambda causes the returned value to be NULL. There is no value in having this selector function. We could directly pass the correct factory. - MotionPlanRequest isn't actually used.
d879aab
to
9e3d71b
Compare
Thank you for spending the time to find my superfluous ampersand that broke the patch... 🏅 |
Fixes #2766. Replaces #2972 .
@Abishalini will hate me for this changeset for its ease of porting to moveit2... 😢
But it was overdue, I managed to do it over a few evenings and I believe it makes the code much more readable.
This PR should get some priority because patches are quite likely to generate conflicts with this due to its size.
I split all non-trivial changes into separate commits with one huge commit in the end addressing all "trivial" (up to identifier naming) cases. During this work I found and reworked a number of internal details, often related to pointer-use where object lifetime was known or APIs that could be reworked with the flexibility of lambdas.
Eventually I added the clang-tidy check that Robert found. It's important to point out that the provided patch will usually work, but ends up with horrible overhead per parameter/capture and no useful identifiers. So users cannot just apply the patch. But then again, nobody will contribute new code that still uses
bind()
anyway... Is what you would hope.@tylerjw @rhaschke