-
Notifications
You must be signed in to change notification settings - Fork 310
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
adding backward compatibility for string-to-double conversion #1284
adding backward compatibility for string-to-double conversion #1284
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1284 +/- ##
==========================================
+ Coverage 47.82% 47.85% +0.03%
==========================================
Files 41 41
Lines 3473 3477 +4
Branches 1888 1889 +1
==========================================
+ Hits 1661 1664 +3
Misses 446 446
- Partials 1366 1367 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Left a couple of comments.
Also, I think it would be cleaner to leave stod()
alone and have two versions of impl::stod()
instead. That way we don't end up changing the API contract when touching individual implementations… what do you think of this?
namespace hardware_interface
{
namespace impl
{
std::optional<double> stod(const std::string & s)
{
#if __cplusplus < 201703L
// Impl with std::istringstream
...
if (stream.fail() || !stream.eof())
{
return std::nullopt;
}
return result;
#else
// Impl with std::from_chars
...
if (parse_result.ec == std::errc())
{
return result_value;
}
return std::nullopt;
#endif
}
} // namespace impl
double stod(const std::string & s)
{
if (const auto result = impl::stod(s))
{
return *result;
}
throw std::invalid_argument("Failed converting string to real number");
}
}
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
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.
LGTM with a couple of comments
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 looks good to me and it seems that the tests are happy too :)
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.
nice, works for me (on debian11 and ubuntu 22.04)
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.
LGTM
@Mergifyio backport iron humble |
✅ Backports have been created
|
--------- Co-authored-by: Sai Kishor Kothakota <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 477c85d)
--------- Co-authored-by: Sai Kishor Kothakota <[email protected]> Co-authored-by: Dr. Denis <[email protected]> (cherry picked from commit 477c85d)
Adding backward compatibility for string to double conversion as mentioned in
#1257 (comment)