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

Add construction tests for Vector2, Vector3, Vector4, Quaternion, and Pose #172

Merged

Conversation

nlamprian
Copy link
Contributor

The classes are already moveable. The error reported in the issue comes from the fact that the operation is a copy assignment to unique_ptr, and not a move assignment.

Closes #76

Signed-off-by: Nick Lamprianidis [email protected]

@github-actions github-actions bot added 🏢 edifice Ignition Edifice 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Oct 26, 2020
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error reported in the issue comes from the fact that the operation is a copy assignment to unique_ptr, and not a move assignment.

Thanks for checking and for the tests. You're right, I checked that the example tests work by changing

ptr2 = ptr1;

to

ptr2 = std::move(ptr1);

src/Vector4_TEST.cc Outdated Show resolved Hide resolved
@nlamprian nlamprian force-pushed the nlamprian/add_construction_tests branch from 4d9d5ca to 7d2c15c Compare October 26, 2020 17:53
@nlamprian
Copy link
Contributor Author

Should these tests be replicated for the rest of the classes?

@chapulina
Copy link
Contributor

Should these tests be replicated for the rest of the classes?

That would be great if you're up for it. Hopefully you won't find any classes that aren't movable. And having the tests in place will make sure we don't break this usage in the future.

But when it comes to #76, I think this is enough to address those concerns, so we can close it.

@chapulina chapulina merged commit 941de79 into gazebosim:ign-math6 Oct 26, 2020
@nlamprian nlamprian deleted the nlamprian/add_construction_tests branch October 26, 2020 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants