Skip to content

Conversation

@rotu
Copy link
Collaborator

@rotu rotu commented Oct 18, 2019

Includes changes in #46, in a way that makes it work both before and after ros2/rmw#187

@rotu rotu changed the title Update for compatibility before and after RMW pub/sub option structures Compatibility before and after RMW pub/sub option structures Oct 18, 2019
Use CMake rmw_VERSION for conditional compilation

Signed-off-by: Dan Rose <[email protected]>
@rotu rotu force-pushed the mmw_version_rebase branch from bac9852 to 6ef7d89 Compare October 18, 2019 17:57
@dirk-thomas
Copy link
Member

The functional changes look good to me. Leaving it up to someone else to merge.

Why are the non-functional changes (line wrapping etc.) part of this PR?

@eboasson
Copy link
Collaborator

Looks good to me, too. Just verified it on current Eloquent on macOS as well as building against an old binary install of dashing. I'll merge if @dirk-thomas is happy with it too.

Many thanks @rotu for helping out with the cyclone RMW and to @dirk-thomas for getting the RMW versioning in so quickly!

@rotu
Copy link
Collaborator Author

rotu commented Oct 18, 2019

Why are the non-functional changes (line wrapping etc.) part of this PR?

I reformatted the code local to my changes to comply with the C++ developer guide. This should not be controversial; please don't bikeshed.

@dirk-thomas
Copy link
Member

This should not be controversial; please don't bikeshed.

The change itself is not controversial. I would just suggest to use a separate PR for unrelated changes - which I don't think is bikeshedding but a commonly agreed upon rational for making pull requests. Anyway - I am not maintaining this repo - so it is just an outsider comment. No need to do anything about it.

@eboasson eboasson merged commit c51c884 into ros2:master Oct 20, 2019
@rotu rotu deleted the mmw_version_rebase branch October 23, 2019 18:29
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.

3 participants