-
Notifications
You must be signed in to change notification settings - Fork 98
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 classes to use ignition::utils::ImplPtr or ignition::utils::UniqueImplPtr #474
Conversation
Signed-off-by: Michael Carroll <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. I don't see anything that sticks out, but it is a bit long to review so it's hard review every single addition.
Like, I'm not sure why sometimes you choose the unique variant or the other.
That is cool though that this addition caught the issue of const-correctness in some cases!
I used the unique variant for |
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
Signed-off-by: Addisu Z. Taddese <[email protected]>
This is needed, otherwise the nightlies will be broken: gazebo-release/sdformat11-release#2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, great work! it took a while to review but only because there were so many files. it was easy enough to click through them quickly because it's mostly deleted code
I think the only thing it's missing is an entry in the Migration guide mentioning the new dependency on ignition-utils1
Signed-off-by: Steve Peters <[email protected]>
|
I didn't expect the const correctness to be too intrusive, but it looks like it broke |
This builds on top of the commits in #447 and updates almost all the classes in libsdformat to use either
ignition::utils::ImplPtr
orignition::utils::UniqueImplPtr
. While making this update, a number of member functions had to be updated for const correctness.Needs: gazebosim/gz-math#187