Broken promise #47
Broken promise #47
Conversation
| if (kv.first.find(prefix + ".") == 0) { | ||
| if (kv.first == prefix) { | ||
| return true; | ||
| } else if (kv.first.find(prefix + ".") == 0) { |
There was a problem hiding this comment.
Looks like there are two spaces between else and if, is that intentional?
There was a problem hiding this comment.
I haven't run uncrustify yet, will do in a moment.
|
Don't review yet. |
| } | ||
|
|
||
| promise_result->set_value(parameter_variants); | ||
| get_parameters_promises_.erase(promise_result); |
There was a problem hiding this comment.
Can you prefix this with this->? I had trouble matching the use of this in the lambda to a specific line.
There was a problem hiding this comment.
Same thing in a few places below.
|
Oh, sorry, I just assumed. It actually looks good to me, other than the minor style comments. |
|
Linux buildfarm: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/161/ |
|
@wjwwood no worries! :-) I noticed I hadn't run uncrustify before submitting the PR. |
|
Please review. |
| if (std::string::npos != last_separator) { | ||
| std::string prefix = kv.first.substr(0, last_separator); | ||
| if (std::find(result.prefixes.cbegin(), result.prefixes.cend(), | ||
| prefix) == result.prefixes.cend()) |
There was a problem hiding this comment.
I found reading this line pretty difficult due to the wrapping within the find call. Wrapping after the comparison operator == helped me to parse the line.
There was a problem hiding this comment.
I also had trouble with that line, but I wasn't sure what to suggest at the time. I think that this is only slightly more readable:
if (std::find(result.prefixes.cbegin(), result.prefixes.cend(), prefix) ==
result.prefixes.cend())
{
/* stuff... */
}
|
|
||
| auto request = std::make_shared<rcl_interfaces::srv::GetParameters::Request>(); | ||
| request->names = names; | ||
|
|
||
| auto self = shared_from_this(); |
There was a problem hiding this comment.
This will only work if the instance is already owned by another shared_ptr right? Do we make that guarantee?
There was a problem hiding this comment.
I'm reworking this, I'll change switch the sets to shared pointers and just pass them down to the lambda.
There was a problem hiding this comment.
Forget what I said, this won't work.
|
Linux buildfarm: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/165/ |
|
+1 |
|
+1 (please squash before merge) |
|
I made a few more changes to avoid storing the promises in the |
|
Waiting on the next Linux Jenkins results before I approve. |
|
Linux buildfarm: http://54.183.26.131:8080/view/ros2/job/ros2_batch_ci_linux/166/ |
|
Much simpler +1. PS @wjwwood now has the new dedicated buildfarm running for ROS 2.0 The job has the same output here: http://ci.ros2.org/job/ros2_batch_ci_linux/8/ |
|
+1 |
|
Rebased against master and squashed the last changes. This builds fine, @jacquelinekay could you give a final review on this? Thanks! |
|
+1. looks good to me. I haven't followed the evolution of the issue/the solution and I don't have a lot of experience using |
|
Thanks everybody for going through this long PR |
* ros2GH-101 Add converter interface * ros2GH-102 Create format converter factory * ros2GH-103 Write documentation for converter plugin authors * ros2GH-16 Adjust rosbag2 message type * ros2GH-16 Change name of converter interface to include "serialization" - Easier to differentiate between storage format (e.g. sqlite) and serialization format (e.g. cdr) - Closer to naming in ros middleware * ros2GH-16 Improve plugin development documentation - Also adapt to name changes * ros2GH-16 Fix naming of SerializationFormatConverterFactory
This PR makes sure that promises are not destroyed too early, adds more cases to the list_parameters verb and fixes the parameters service to listen on the appropriate topic.
Fixes #44