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

Update KDL SegmentMap interface to use shared pointers #5

Merged
merged 1 commit into from
Jul 21, 2014
Merged

Update KDL SegmentMap interface to use shared pointers #5

merged 1 commit into from
Jul 21, 2014

Conversation

jensenb
Copy link
Contributor

@jensenb jensenb commented Nov 19, 2013

This is a dependent change for the orocos KDL API modification discussed in: orocos/orocos_kinematics_dynamics#4

To summarize the KDL API used incomplete type definitions in STL containers, which is undefined behaviour according the c++ standard. As it was, GCC / libstc++ handles this behaviour with problems; however, on systems with clang/libc++ using incomplete type definitions in STL containers results in compilation errors. Instead a solution involving shared pointers has been proposed. More rational the change is discussed in the upstream pull request.

The upstream pull request is necessary for this change to be merged.

@wjwwood
Copy link
Member

wjwwood commented Nov 20, 2013

@jensenb I don't think this can be released into hydro until the updated version of orocos_kdl is released into hydro, will it be an API breaking change?

In order to merge this now, I would want to see a backwards compatible change, potentially using #ifdef's to have two code paths for different versions of orocos_kdl. If not this then we should probably target this release for indigo (next version of ROS).

Also, if this will only work with newer versions then this package (or kdl_parser) should have a versioned dependency on orocos_kdl.

@jensenb
Copy link
Contributor Author

jensenb commented Nov 21, 2013

Yes the orocos_kdl upstream change will mandate a (slight) API change. From my perspective, there really is no way around this. Their self referential tree data structure has undefined behaviour, and while it works currently fine with gcc and libstc++, it is not guaranteed to work with all standards conforming c++ compiler and standard library implementations, nor is it guaranteed to work with newer versions of gcc/libstdc++.

I opened this change request so that:

  • the KDL guys could see the downstream effects of the API change
  • Mavericks users could pull this change and at least get hydro to compile on their systems.

I don't know how much code uses KDL, but I think it would be cleaner and much more maintainable if only one API version was supported. Once the KDL guys have decided how they want to handle the problem and committed the change with proper version info, then the development branch could just switch to the new version using this change. I mean, does the devel branch and do future releases of robot_state_publisher and robot_model really have to support an older version of KDL? People who the need the change now (Mavericks users), will just have to use devel branch until the next ROS release.

I would hold on on this change until the KDL team decides what to do about the problem.

@wjwwood
Copy link
Member

wjwwood commented Nov 21, 2013

I agree that we wait to see what the Orocos developers do, please ping back here when there is progress, otherwise it will likely fall through the cracks.

The KDL Tree API optionally uses shared pointers on platforms where
the STL containers don't support incomplete types.
@jensenb
Copy link
Contributor Author

jensenb commented Mar 5, 2014

Updated this PR to reflect the newest changes in the upstream pull request.

@jensenb
Copy link
Contributor Author

jensenb commented May 28, 2014

I completely forgot to check up on the PR, is there anything holding this back currently from being merged into hydo-devel and indigo-devel?

@NikolausDemmel
Copy link

bump

EDIT: The bump is about getting these changes released into hydro/indigo in a coordinatd fashion: ros/robot_model#43, #5, #4, #19

@NikolausDemmel
Copy link

Now that orocos_kdl has been re-released (https://github.com/smits/orocos-kdl-release) including the patch that this PR depends on, can we merge this, forward-port the patch to indigo, and then possibly some time get a hydro and an indigo release?

@wjwwood
Copy link
Member

wjwwood commented Jul 21, 2014

Same comment here as with ros/robot_model#43, @isucan I can do this for you if you're ok with the change.

isucan added a commit that referenced this pull request Jul 21, 2014
Update KDL SegmentMap interface to use shared pointers
@isucan isucan merged commit 8717c07 into ros:hydro-devel Jul 21, 2014
@isucan
Copy link
Contributor

isucan commented Jul 21, 2014

Thanks @wjwwood !

clalancette pushed a commit that referenced this pull request Mar 22, 2019
* Windows bringup.

* remove STATIC and binplace to correct location. (#2)

* add space after if keyword (#3)

* update install destinations (#4)

* update install destinations

* remove ARCHIVE and LIBRARY deestinations

* only add -Wall if using MSVC (#5)

* always add -Wall -Wextra flags

* only add -Wall if using MSVC

* separate out compiler flag change
clalancette pushed a commit that referenced this pull request Mar 22, 2019
* only add c++11 flag when it's supported

* add space after if keyword (#3)

* only add -Wall if using MSVC (#5)

* always add -Wall -Wextra flags

* only add -Wall if using MSVC

* update how compiler flags are added (#6)
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.

4 participants