-
Notifications
You must be signed in to change notification settings - Fork 82
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
[FEATURE] Add to_chars overload for floating points. #1160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1160 +/- ##
=======================================
Coverage 96.36% 96.36%
=======================================
Files 201 201
Lines 7782 7782
=======================================
Hits 7499 7499
Misses 283 283 Continue to review full report at Codecov.
|
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 good! I have just two questions where things are not clear to me:
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.
Well done!
@rrahn kind review ping :) this is a tiny PR that I would like to get in ASAP as another one depends on it. |
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.
Some minor things
template <seqan3::FloatingPoint floating_point_type> | ||
inline std::to_chars_result to_chars(char * first, char * last, floating_point_type value) noexcept | ||
{ | ||
std::ostringstream ss; |
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.
Please assert that both ptr values are not nullptr
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.
Maybe in 2 separate asserts so you can see by the line number which one failed.
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.
fine 🙄
test/unit/std/charconv_test.cpp
Outdated
{ | ||
TypeParam val{120}; | ||
char buffer[10]; | ||
|
||
// buffer.clear(); | ||
[[maybe_unused]] auto res = std::to_chars(&buffer[0], &buffer[0] + sizeof(buffer), val); | ||
auto res = std::to_chars(&buffer[0], &buffer[0] + sizeof(buffer), val); |
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 recommend using std::array instead since it would be the c++ way.
template <seqan3::FloatingPoint floating_point_type> | ||
inline std::to_chars_result to_chars(char * first, char * last, floating_point_type value) noexcept | ||
{ | ||
std::ostringstream ss; |
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.
Maybe in 2 separate asserts so you can see by the line number which one failed.
No description provided.