Avoiding default atof behavior for integers and floats #373
Conversation
| long long int converted = std::strtoll(t.c_str(), &end, 10); | ||
| if (*end != '\0') { | ||
| // was not at end of element so throw error | ||
| throw std::runtime_error("Invalid conversion: String contains more characters than just integer"); |
There was a problem hiding this comment.
Let's put the value into the error message, so the user has context on why it failed:
"Invalid conversion: \"" + t + "\" cannot be interpreted as an integer."Same for the float conversion below.
There was a problem hiding this comment.
That's a great idea, on it!
|
@shantanuchhabra - a unit test would be great. I would recommend creating a simple unit test. Verify that it fails without your change. Then verify that it passes with your change. |
|
Changed the error message and also added a unit test! |
| long long int converted = std::strtoll(t.c_str(), &end, 10); | ||
| if (*end != '\0') { | ||
| // was not at end of element so throw error | ||
| throw std::runtime_error("Invalid conversion: " + t + "cannot be interpreted as an integer"); |
There was a problem hiding this comment.
probably want a space before "cannot"
| double converted = std::strtod(t.c_str(), &end); | ||
| if (*end != '\0') { | ||
| // was not at end of element so throw error | ||
| throw std::runtime_error("Invalid conversion: " + t + "cannot be interpreted as a float"); |
There was a problem hiding this comment.
same as previous comment
… unity_sarray and hence gets silenced by the error message in unity_sarray on line 1336/1340?
|
Hi, sorry. My review came late. Can we revert? |
This reverts commit 7518bf2.
| inline FLEX_ALWAYS_INLINE_FLATTEN flex_int operator()(const flex_string& t) const { | ||
| char *end; | ||
| long long int converted = std::strtoll(t.c_str(), &end, 10); | ||
| if (*end != '\0') { |
There was a problem hiding this comment.
I rather this not use (*end) == '\0' as a comparison. The null character could occur anywhere.
Use string.data(), and check for position of the pointer instead of comparing the value at the pointer.
| inline FLEX_ALWAYS_INLINE_FLATTEN flex_float operator()(const flex_string& t) const { return std::atof(t.c_str()); } | ||
| inline FLEX_ALWAYS_INLINE_FLATTEN flex_float operator()(const flex_string& t) const { | ||
| char *end; | ||
| double converted = std::strtod(t.c_str(), &end); |
| @@ -1300,9 +1300,9 @@ std::shared_ptr<unity_sarray_base> unity_sarray::lazy_astype(flex_type_enum dtyp | |||
| flexible_type ret; | |||
| try { | |||
| if (dtype == flex_type_enum::INTEGER) { | |||
| ret = std::stoll(f.get<flex_string>()); | |||
| f.to<flex_int>(); | |||
There was a problem hiding this comment.
to<...> returns. It does not cast inplace. See the flexible_type documentation.
There was a problem hiding this comment.
Of course - not sure how I missed that. How does this pass unit tests?
|
@shantanuchhabra Let's address @ylow's comments. You can either start fresh (I'll git revert this PR change) or you can put up a 2nd PR to address the comments - let me know which you'd prefer. |
|
I'll create a second PR! |
|
Please see #376 :) |
Redirected from unity_sarray to go to flex_int and flex_float and added logic in flexible_type_detail to deal with invalid strings like "2hello", "INFiltrate", etc. Built and tested locally.