Add ClassLoader::createUniqueInstance #38

de-vri-es · 2016-09-11T09:04:00Z

This PR adds a function to create managed instances wrapped in a std::unique_ptr.

There are a number of advantages to unique_ptr:

It accurately models the fact that you are the sole owner when you create the instance.
It can be implicitly converted to std::shared_ptr and boost::shared_ptr (since boost 1.53).
It can release the held pointer to be used in whichever ownership or lifetime management system the user wants.

The header hides the #include <memory> and the extra function if C++11 support is not available. That means dependent projects are not forced to enable C++11 support. Since the added function is actually a function template, there isn't even a need to enable C++11 support for compiling ClassLoader. As such it is even safe to merge this into indigo.

Since std::unique_ptr is implicitly convertible to boost::shared_ptr I was also contemplating just changing the return type of ClassLoader::createInstance. It would be mostly API compatible. However, it would not ABI compatible and there are cases where it would not be API backwards compatible either (if someone is using an implicit conversion on the returned boost::shared_ptr that is not available for std::unique_ptr, for example). Doing this may make sense in the future, but for now this seemed the most pragmatic approach.

davetcoleman · 2016-09-11T16:07:14Z

Thanks @de-vri-es. This will help us upgrade MoveIt! to C++11

davetcoleman · 2016-09-11T16:08:20Z

include/class_loader/class_loader.h

+    template <class Base>
+    std::unique_ptr<Base> createUniqueInstance(const std::string& derived_class_name)
+    {
+      return boost::shared_ptr<Base>(createRawInstance<Base>(derived_class_name, true));


Why is this still using a boost::shared_ptr?

de-vri-es · 2016-09-11T19:04:31Z

I forgot the deleters, fixed now. Tests pass locally.

de-vri-es · 2016-09-11T21:26:08Z

I should also update MultiLibraryClassLoader.

de-vri-es · 2016-09-11T21:52:03Z

Should also extend unit tests for the unique_ptr version.

davetcoleman · 2016-09-12T17:55:00Z

@mikaelarguedas think you can look at this?

mikaelarguedas · 2016-09-12T18:02:46Z

Started reading it but will do a more thorough review very soon

mikaelarguedas · 2016-09-14T05:15:26Z

looks good to me.
Thanks for keeping all the c++11 specific stuff inside ifdefs to keep c++11 support optional.
Agreed to keep the current return type of ClassLoader::createInstance to boost::shared_ptr it is better for now to avoid any ABI/API breakage.
We should definitely take advantage more of the c++11 features in the future (starting with ROS-L?), I've just been trying to avoid branching out and maintaining several version of the same code for core packages.

davetcoleman · 2016-09-14T07:53:06Z

Thanks for reviewing @mikaelarguedas. Do you think this could be released by the next kinetic sync?

de-vri-es · 2016-09-14T08:08:34Z

Note I'm working on a unit test now which revealed some problems with the unique_ptr API. Will update the PR within the hour.

de-vri-es · 2016-09-14T08:32:34Z

Unit test added.

Also changed the unique_ptr deleter to an std::function rather than the result of bind directly. It keeps the unique_ptr a lot simpler. It makes the unique_ptr default constructible and implicitly convertible from nullptr. It also makes the type a whole lot easier to specify. It possibly comes at the cost of a slight performance penalty, but I think that is worth the trade-off here.

mikaelarguedas · 2016-09-19T17:22:00Z

Thanks @de-vri-es for adding the unit test and cleaning up branching.
@davetcoleman I'll perform some more testing before +1ing this, ths buildfarm is currently rebuilding all kinetic so this will be available for the next sync after the current one, certainly a couple of weeks from now.

mikaelarguedas · 2016-09-19T17:14:46Z

test/unique_ptr_test.cpp

+  ClassLoader loader1(LIBRARY_1);
+  ASSERT_TRUE(loader1.isLibraryLoaded());
+
+  //Note: Hard to test thread safety to make sure memory isn't corrupted. The hope is this test is hard enough that once in a while it'll segfault or something if there's some implementation error.


Agreed that it's hard to test. Will try it locally with a lot of threads to make sure it passes every time but a thousand seems to be definitely hard enough to unveil any threading issue

Note that I copied this from the shared_ptr test. It's somewhat double, since this test is actually testing if the deleter behaves correctly.

Both the shared and unique pointers have the same deleter though, so unless the unique_ptr implementation is buggy (which is unlikely), the behaviour should be the same. Still, I figured it can't hurt to test this with the unique_ptr too.

Fair point thanks for the explaination.
I couldn't produce any failure locally either. Agreed that it can't hurt to keep the test.

mikaelarguedas · 2016-09-19T17:17:11Z

include/class_loader/multi_library_class_loader.h

-      throw(class_loader::CreateClassException("MultiLibraryClassLoader: Could not create object of class type " + class_name + " as no factory exists for it. Make sure that the library exists and was explicitly loaded through MultiLibraryClassLoader::loadLibrary()"));
+      ClassLoader* loader = getClassLoaderForClass<Base>(class_name);
+      if (loader == NULL)
+        throw class_loader::CreateClassException("MultiLibraryClassLoader: Could not create object of class type " + class_name + " as no factory exists for it. Make sure that the library exists and was explicitly loaded through MultiLibraryClassLoader::loadLibrary()");


I know it's a copy paste from previous error throwing but would you mind wrapping these lines to compile with google styleguide ? it can also be done in a following style PR to address all the occurences of the lack of wrapping in classloader

I can do that, but there is something to be said for not breaking up user-visible error messages (to quickly point them to the relevant bit of code when they grep).

There are some natural break-up points (variables and punctuation) that can be used without making grep useles, but I'm not sure that will stay within a strict 80 character line limit in all cases.

That's a very good point, let's keep it as is given that it's consistent with every other error message in this package

mikaelarguedas · 2016-09-19T22:26:51Z

test/CMakeLists.txt

 endif()
+
+catkin_add_gtest(${PROJECT_NAME}_unique_ptr_test unique_ptr_test.cpp)
+if(TARGET ${PROJECT_NAME}_unique_ptr_test)


this test is never triggered because by default c++11 flag is not set for any rosdistro (c.f. http://build.ros.org/job/Kpr__class_loader__ubuntu_xenial_amd64/19/console#console-section-17)
Could you test if c++11 is available and set the flag only for this test? using this kind of logic:

include(CheckCXXCompilerFlag) CHECK_CXX_COMPILER_FLAG("-std=c++11" COMPILER_SUPPORTS_CXX11) if(COMPILER_SUPPORTS_CXX11) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11") endif()

Thanks!

Good point, I'm used to working with GCC 6 which did compile the test (it defaults to -std=gnu++14).

Test updated: removed the #ifdef in the test and instead made the compilation conditional in the CMakeLists.txt with the flags added if supported by the compiler.

Thanks for iterating on this, tests are compiled and run properly now.

de-vri-es · 2016-09-20T18:29:09Z

Thanks for the thorough review :)

/edit: And now on to pluginlib for me ;)

mikaelarguedas · 2016-09-20T18:34:35Z

My pleasure, especially when there is nothing to complain about like on this PR :)
Thanks for contributing!

mikaelarguedas · 2016-09-20T22:44:15Z

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

* manual port of #38 * add static private setter function for unmanaged flag * missing newline * use NULL unless within a "if C++11" block * fixups

mikaelarguedas · 2017-11-13T06:26:22Z

@de-vri-es I noticed that the added test file doesnt have a copyright notice. Is it possible to provide a follow up PR adding it with the appropriate copyright holder? thanks!

de-vri-es · 2017-11-13T09:13:48Z

Good point, will do it now. Going to be my employer. Although I see I copied a lot.

de-vri-es mentioned this pull request Sep 11, 2016

C++11ifying the public API for the kinetic release moveit/moveit#48

Closed

davetcoleman reviewed Sep 11, 2016
View reviewed changes

de-vri-es force-pushed the unique_ptr branch 2 times, most recently from ff1e6bf to 42b4106 Compare September 11, 2016 19:03

de-vri-es force-pushed the unique_ptr branch 2 times, most recently from 8975ee8 to 6536c49 Compare September 11, 2016 22:14

davetcoleman mentioned this pull request Sep 12, 2016

Convert API to use std::shared_ptr moveit/moveit#215

Merged

de-vri-es added 2 commits September 14, 2016 10:22

Wrap comments on createInstance and friend.

fb657cb

Delegate createInstance and createUnmanagedInstance to private impl.

5f0fc64

de-vri-es force-pushed the unique_ptr branch from 6536c49 to fab4684 Compare September 14, 2016 08:23

Add ClassLoader::createUniqueInstance.

70eacc2

de-vri-es force-pushed the unique_ptr branch from fab4684 to c73f783 Compare September 14, 2016 08:28

de-vri-es added 2 commits September 14, 2016 10:36

MultiLibraryClassLoader: Factor out getClassLoaderForClass.

267e68d

MultiLibraryClassLoader: Add unique_ptr API.

366e118

de-vri-es force-pushed the unique_ptr branch from c73f783 to 27c468b Compare September 14, 2016 08:37

mikaelarguedas requested changes Sep 19, 2016

View reviewed changes

mikaelarguedas added the in progress label Sep 20, 2016

Add tests for unique_ptr API.

7705792

de-vri-es force-pushed the unique_ptr branch from 27c468b to 7705792 Compare September 20, 2016 18:08

mikaelarguedas approved these changes Sep 20, 2016

View reviewed changes

mikaelarguedas merged commit ca6a612 into ros:indigo-devel Sep 20, 2016

mikaelarguedas removed the in progress label Sep 20, 2016

de-vri-es mentioned this pull request Sep 20, 2016

Add std::unique_ptr API ros/pluginlib#42

Merged

wjwwood added a commit that referenced this pull request Oct 26, 2017

manual port of #38

4f11ec6

wjwwood mentioned this pull request Oct 26, 2017

manual port of unique ptr pr #58

Merged

mikaelarguedas pushed a commit that referenced this pull request Oct 31, 2017

manual port of unique ptr pr (#58)

adcb526

* manual port of #38 * add static private setter function for unmanaged flag * missing newline * use NULL unless within a "if C++11" block * fixups

de-vri-es mentioned this pull request Nov 13, 2017

Add copyright notice to unique_ptr_test.cpp #65

Merged

Add ClassLoader::createUniqueInstance #38

Add ClassLoader::createUniqueInstance #38

Uh oh!

Conversation

de-vri-es commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davetcoleman commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-vri-es commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

de-vri-es commented Sep 11, 2016

Uh oh!

de-vri-es commented Sep 11, 2016

Uh oh!

davetcoleman commented Sep 12, 2016

Uh oh!

mikaelarguedas commented Sep 12, 2016

Uh oh!

mikaelarguedas commented Sep 14, 2016

Uh oh!

davetcoleman commented Sep 14, 2016

Uh oh!

de-vri-es commented Sep 14, 2016

Uh oh!

de-vri-es commented Sep 14, 2016

Uh oh!

mikaelarguedas commented Sep 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-vri-es Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

de-vri-es commented Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikaelarguedas commented Sep 20, 2016

Uh oh!

mikaelarguedas commented Sep 20, 2016

Uh oh!

mikaelarguedas commented Nov 13, 2017

Uh oh!

de-vri-es commented Nov 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

de-vri-es commented Sep 11, 2016 •

edited

Loading

davetcoleman commented Sep 11, 2016 •

edited

Loading

de-vri-es commented Sep 11, 2016 •

edited

Loading

de-vri-es Sep 20, 2016 •

edited

Loading

de-vri-es commented Sep 20, 2016 •

edited

Loading

de-vri-es commented Nov 13, 2017 •

edited

Loading