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

implement safer align function #186

Merged
merged 1 commit into from
May 27, 2020
Merged

Conversation

dodsonmg
Copy link
Contributor

@dodsonmg dodsonmg commented May 20, 2020

avoids unnecessary casts to pointer and pointer manipulation by reference.

this is the same as PR #141, which was previously merged into master, but way ahead of the dashing-eloquent fork.

@dodsonmg
Copy link
Contributor Author

dodsonmg commented May 20, 2020

Looks like Windows test failed while acquiring dependencies... I can try to rerun by pushing again?

@rotu
Copy link
Collaborator

rotu commented May 20, 2020

@dodsonmg No need. I need to backport #181 which uses setup-ros@master to fix this bug: ros-tooling/setup-ros#156

@thomas-moulard time to cut a new release or recommend people use master? https://github.com/ros-tooling/setup-ros/blob/master/README.md#usage

@dodsonmg
Copy link
Contributor Author

dodsonmg commented May 22, 2020

For my own planning purposes: are you waiting for the Windows CI to be fixed before considering the merge (same question for #189)?

@rotu
Copy link
Collaborator

rotu commented May 22, 2020

For my own planning purposes: are you waiting for the Windows CI to be fixed before considering the merge (same question for #189)?

Sorry, please don't wait on me. I'm taking a break from Cyclone stuff (indefinite but hopefully not long). I defer to @eboasson.

@eboasson
Copy link
Collaborator

@dodsonmg any reason this isn't a cherry-pick? It looks to me like the change is identical, and keeping the metadata intact would be better.

I don't think there is a need to wait for the windows build failure in the CI to be addressed given that the changes are fine on Foxy and don't interact with the ROS libraries. But if, e.g., @nuclearsandwich or @jacobperron says differently, then it is different.

@dodsonmg
Copy link
Contributor Author

Sure. I just don't know how to do it.

avoids unnecessary casts to pointer and pointer manipulation by reference
@dodsonmg
Copy link
Contributor Author

Cherry picking seems to have worked. Windows build still fails, and for some reason the MacOS build was cancelled... I can push again to see if I can dislodge the MacOS build?

@dodsonmg
Copy link
Contributor Author

Any other feedback on this one (i.e., advice for getting through the cancelled builds)?

Same for #189.

@eboasson
Copy link
Collaborator

@dodsonmg no further feedback other than thanks. Given the nature of changes, the fact that the build failures here are unrelated and that it works fine on Foxy, I am good with merging it. The delay is just the result of having too much to do ...

@eboasson eboasson merged commit 1ffb39f into ros2:dashing-eloquent May 27, 2020
@dodsonmg dodsonmg deleted the align_types branch May 29, 2020 15:40
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.

None yet

3 participants