-
Notifications
You must be signed in to change notification settings - Fork 523
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
PRBT IkFast patch from robostack #2395
Conversation
As long as a dynamic memory allocation is ok in this context, this seems ok. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2395 +/- ##
==========================================
+ Coverage 50.78% 50.80% +0.02%
==========================================
Files 386 386
Lines 31972 31972
==========================================
+ Hits 16235 16239 +4
+ Misses 15737 15733 -4 ☔ View full report in Codecov by Sentry. |
I don't understand how it could have worked before without dynamic memory allocation so I have to assume it is ok. C variable length array was using dynamic memory too. |
As far as I know (but I do not know in-depth) VLA at least in GCC use the stack, see https://gcc.gnu.org/onlinedocs/gcc/Variable-Length.html (the referenced function |
(cherry picked from commit bd45079)
Description
Modified version of this: https://github.com/RoboStack/ros-humble/blob/main/patch/ros-humble-moveit-resources-prbt-ikfast-manipulator-plugin.patch
Quoting @traversaro
I used a std::vector as we tend to use C++ types to avoid raw new/delete here in MoveIt.
On the version of ikfast changing in that static_assert. I searched for copies of the ikfast.h header, and it is in two places in our repo, and in both places, the version is 61. When I set the version to what it was in that hack, it failed, complaining that the version is 61 so the old test wasn't working anyway, and we've been compiling with version 61.