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

Compatibility before and after RMW pub/sub option structures #51

Merged
merged 2 commits into from
Oct 20, 2019

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

Signed-off-by: Erik Boasson <eb@ilities.com>
@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 <dan@digilabs.io>
@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