-
Notifications
You must be signed in to change notification settings - Fork 418
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are two spaces between else
and if
, is that intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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... */
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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