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 std::unique_ptr API #42

Merged
merged 4 commits into from
Sep 20, 2016
Merged

Conversation

de-vri-es
Copy link
Contributor

This follows ros/class_loader#38 and adds the API to pluinglib as well. That means it requires indigo-devel from class_loader to build, which is probably not available for the automated tests. I'm not sure what the best solution is for that. It would be nice to release this API addition together with class_loader.

Points to note for the review:

  • I put the unit test in a separate file. I was tempted to put them in the same file with the other tests, but that would have required compiling those tests with C++11 support as well. I figured it was best not to do that, to make sure no hard requirements on C++11 features sneak in where they shouldn't.
  • I switched manual try/catch statements in the unit test with ASSERT_THROW for readability. ASSERT_THROW checks the exception type, so old behaviour of the test is maintained.
  • I also cleaned up some white space errors and long unwrapped comments. I can undo those if desired.

@mikaelarguedas
Copy link
Member

Thanks for the follow up PR, the change looks good for me.
I'll test them locally on the different distros and release them together to run the tests all at once on the buildfarm.
It's ok to have the cleanup (whitespace/wrapping) commit in the same PR.
Thanks for replicating the c++11 check for compiling the tests.
Will comment here once tested locally

Copy link
Contributor

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

I skimmed the changes

@mikaelarguedas
Copy link
Member

tested locally on both kinetic and indigo without problem

@mikaelarguedas mikaelarguedas merged commit 1cc04a8 into ros:indigo-devel Sep 20, 2016
@de-vri-es
Copy link
Contributor Author

Thanks for the quick review and merge :)

@mikaelarguedas
Copy link
Member

Released on kinetic and will release on earlier distros once we're sure that no unexpected behaviour emerges

@v4hn
Copy link

v4hn commented Sep 29, 2016

It would be great to have this available in indigo too! So +1 to backport this change to indigo and jade

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

4 participants