Provide defaults for ParameterValue fields #18
Conversation
Even though only one field for a particular parameter is intended to be used, to prevent serialisation of uninitialised fields we put defaults for all of them
| int64 integer_value 0 | ||
| float64 double_value 0.0 | ||
| string string_value "" | ||
| byte[] bytes_value [] |
There was a problem hiding this comment.
The default value for the array is unnecessary. An array is always empty by default.
| int64 integer_value | ||
| float64 double_value | ||
| string string_value | ||
| bool bool_value False |
There was a problem hiding this comment.
Please use false(lowercase) to match the design specification
There was a problem hiding this comment.
to clarify, is your comment just for style? or is False not supposed to work? (it seems to work)
There was a problem hiding this comment.
It is not supposed to work given that our specification defines true, false, 1 and 0 as the only allowed valued for bool. It happens to work because we account for user error in our parser by lowercasing the value. But it is an implementation detail and would not work for any other parser written following the spec so I think we should not use it in our messages
There was a problem hiding this comment.
ok thanks for the explanation -- just wanted to check we know why it's working as is. fixed in 43081b3
| @@ -6,8 +6,8 @@ | |||
| uint8 type | |||
There was a problem hiding this comment.
Actually should we initialize this too?
There was a problem hiding this comment.
yeah good point, I guess that PARAMETER_NOT_SET would be a good default. Not sure how we refer to message constants from another msg file though.
There was a problem hiding this comment.
Hm, I guess we can't at the moment...
There was a problem hiding this comment.
My reasoning for not putting a default for this was that, while all-but-one of the _value fields will be unused, the type is not "optional" so the client library should always initialise it itself
There was a problem hiding this comment.
Hm, I guess we can't at the moment...
Should we open a ticket for this somewhere (rosidl?) that would be a useful feature
There was a problem hiding this comment.
I've got two thumbs up on my last comment so will consider this question resolved. (in the future, reactions don't send notifications (at least not to me!) so comments would be appreciated)
There was a problem hiding this comment.
👍 sorry forgot about that, yeah it would be great if github allowed to enable notification for "reactions"
|
I was running some tests on other PRs and wanted to know that this wasn't on master for the CI. I'll merge this now. Since @mikaelarguedas pointed out that the default values we specify here are the same as the defaults for those message types in general, I've opened #19 to track that this can be reverted if/when default-initialised messages can be created in client libraries |
This reverts commit d25c83b.
Even though only one field for a particular parameter is intended to be used at a time, to prevent serialisation of uninitialised fields we put defaults for all of them.
This is an alternative to ros2/rclcpp#358, both designed to fix the valgrind error referenced in that PR.