Allow using NaN default values #825
Conversation
henrygerardmoore
requested review from
adityapande-1995,
methylDragon,
quarkytale and
sloretz
as code owners
Signed-off-by: Henry Moore <henrygerardmoore@gmail.com>
henrygerardmoore
force-pushed
the
add_nan
branch
from
8256d77
to
5d424cc
Compare
Signed-off-by: Henry Moore <henrygerardmoore@gmail.com>
Signed-off-by: Henry Moore <henrygerardmoore@gmail.com>
henrygerardmoore
force-pushed
the
add_nan
branch
from
b8b4d41
to
1447047
Compare
Signed-off-by: Henry Moore <henrygerardmoore@gmail.com>
henrygerardmoore
force-pushed
the
add_nan
branch
from
6cd880b
to
75b47b0
Compare
There was a problem hiding this comment.
@henrygerardmoore thank you for the PR.
IMO we can revise #789 or borrow some fixes from there? what do you think?
Signed-off-by: Henry Moore <henrygerardmoore@gmail.com>
|
@fujitatomoya Thank you for the review! Apologies that I didn't see #789 before making this, I should've checked for similar existing PRs. I have applied your suggestions, they both made sense and seemed correct to me. |
yea, compared to my PR, it's very similar - looks good. I never got an answer from the ROS maintainers on how they want to handle equality of messages that contain NaN fields. The lack of consenses is what held me up spending more time on this. |
|
@Ryanf55 thank you for taking a look! I can close this PR now that I am aware of yours. |
| @@ -209,9 +222,13 @@ def basic_value_to_c(type_, value): | |||
| return f'{value}ull' | |||
|
|
|||
| if 'float' == type_.typename: | |||
| if isnan(float(value)): | |||
| return 'NAN' | |||
There was a problem hiding this comment.
I chose to use 'nanf(\"\")' on my PR to avoid the c-style cast to a double later.
| @[ end if]@ | ||
| @[ elif constant.type.typename == 'double']@ | ||
| @[ if constant.value == 'nan'] | ||
| std::numeric_limits<double>::quiet_NaN()@ |
There was a problem hiding this comment.
Instead of coding this, I chose to use the primitive_value_to_cpp function to promote reuse.
| 'octet', | ||
| 'int8', 'uint8', | ||
| 'int16', 'uint16', | ||
| ]: | ||
| return str(value) | ||
|
|
||
| if type_.typename in ['double', 'long double']: |
| 'octet', | ||
| 'int8', 'uint8', | ||
| 'int16', 'uint16', | ||
| ]: | ||
| return str(value) | ||
|
|
||
| if type_.typename in ['double', 'long double']: | ||
| if isnan(value): | ||
| return 'std::numeric_limits<double>::quiet_NaN()' |
There was a problem hiding this comment.
Instead of hard-coding as double, I used the type name here.
| 'octet', | ||
| 'int8', 'uint8', | ||
| 'int16', 'uint16', | ||
| ]: | ||
| return str(value) | ||
|
|
||
| if type_.typename in ['double', 'long double']: | ||
| if isnan(value): |
There was a problem hiding this comment.
For some reason, I needed to surroudn value in a float() cast otherwise it threw.
Yea, please review mine and see if you can find any issues. A lot of the content is very similar. |
Hello, this is my first attempt at a contribution here, so please let me know if I need to change something.
Anyway, I noticed that you cannot default to NaN in
.msgdefinitions, which could be useful, especially when NaNs are used as a sentinel value, which is somewhat common. This PR is my attempt to add this capability.Is the docstring I added correct? The
rosidl_generator_cppprimitive_value_to_cppfunction says that the type of value is a python builtin, but when I triedisnaninrosidl_generator_cit didn't work, while== 'nan'did. I'm not sure how to testrosidl_generator_cppso I would appreciate a double check or someone telling me how to test it.Oh also, I tried adding a bogus message in the
.msgfile (float32 FLOAT32_FAKE=test) to make sure it wouldn't just convert to NaN, and it didn't. Just failed to build, as you would expect.