manual port of unique ptr pr #58
Conversation
wjwwood
changed the title
|
Travis is failing due to poco not building with an error about the c++14 flag. I'll run ci.ros2.org CI instead.
|
|
Finally figured out the issue on Windows (I think). |
|
|
@mikaelarguedas I still don't know about the travis ci, but the other CI is green. Can you re-review? |
|
Yep will do, not sure I'll have time to look at it today though.
Yeah it appeared on other PRs too (some discussion here), I didnt have time to look into it but it's not due to this change 👍 |
There was a problem hiding this comment.
Thanks @wjwwood for the painful port! (I feel you), I may have one or two comments so I'll wait until tomorrow to finish reviewing and merging.
| return *i; | ||
| } | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
looks like sometime you use NULL to match the ROS1 version and sometimes nullptr. Any reason this ones uses another one than the one from upstream?
There was a problem hiding this comment.
That's just habit (to use nullptr). It's better to use than NULL, but also C++11 only:
http://en.cppreference.com/w/cpp/language/nullptr
I'll replace this with NULL for now, so it's closer to the ROS 1 style.
There was a problem hiding this comment.
I left two which are within a c++11 block and so they can use nullptr safely.
There was a problem hiding this comment.
Thanks @wjwwood for iterating. Change looks good to me, I have one question inline about some mismatch in documentation.
I don't mind fixing the nitpicks myself
| * @param derived_class_name The name of the class we want to create (@see getAvailableClasses()) | ||
| * It is not necessary for the user to call loadLibrary() as it will be | ||
| * invoked automatically if the library is not yet loaded (which typically | ||
| * happens when in "On Demand Load/Unload" mode). |
There was a problem hiding this comment.
looking at the upstream PR, some comment content has not been propagated:
* Creating an unmanaged instance disables dynamically unloading libraries when
* managed pointers go out of scope for all class loaders in this process.
| } | ||
|
|
||
| /** | ||
| * @brief Creates an instance of an object of given class name with ancestor class Base |
There was a problem hiding this comment.
Nit: all other functions in this PR have the brief outside of the commented block with ///
include/class_loader/class_loader.h
Outdated
| /** | ||
| * It is not necessary for the user to call loadLibrary() as it will be | ||
| * invoked automatically. | ||
| * If the library is not yet loaded (which typically happens when in |
There was a problem hiding this comment.
Nit: I think this is the continuation of the previous sentence and not a new one (same below)
include/class_loader/class_loader.h
Outdated
| /** | ||
| * It is not necessary for the user to call loadLibrary() as it will be | ||
| * invoked automatically. | ||
| * if the library is not yet loaded (which typically happens when in |
There was a problem hiding this comment.
Nit: I think this is the continuation of the previous sentence and not a new one
|
No worries, I think I addressed all the nitpicks. |
👍 looks good to me, merging |
|
@wjwwood FYI: Big oversight on my side, I mostly compared the changes to the ported ROS1 PR but didnt take into account the ROS2 context. This new test require both |
|
Oops, my bad too. I didn't even think about it after it compiled and ran some tests which passed :/ |
manual port of #38
This was painful 😄. I think
class_loaderneeds to be brought closer to the ROS 1 version as I've been trying to do withpluginlib, but for now this lets me move forward.