-
Notifications
You must be signed in to change notification settings - Fork 93
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
fixing fast float stod conversion according to ST_Number schema specification #339
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #339 +/- ##
========================================
Coverage 69.19% 69.20%
========================================
Files 271 271
Lines 28688 28691 +3
========================================
+ Hits 19851 19855 +4
+ Misses 8837 8836 -1 ☔ View full report in Codecov by Sentry. |
Tests/CPP_Bindings/Source/Reader.cpp
Outdated
@@ -149,4 +149,10 @@ namespace Lib3MF | |||
auto reader = model->QueryReader("3mf"); | |||
ASSERT_SPECIFIC_THROW(reader->ReadFromFile(sTestFilesPath + "/Reader/" + "GEN-M-ADA-ITEM-TRANSFORM-0.3mf"), ELib3MFException); | |||
} | |||
|
|||
|
|||
TEST_F(Reader, IntegrationTestError) { |
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.
good idea to add a test there.
Please explain what the content of the file is and why it should fail.
Also please change the name to something meaningfull.
Source/Common/NMR_StringUtils.cpp
Outdated
@@ -161,6 +161,10 @@ namespace NMR { | |||
{ | |||
throw CNMRException(NMR_ERROR_EMPTYSTRINGTODOUBLECONVERSION); | |||
} | |||
else if (answer.ptr && answer.ptr[0] == ',') // Invalidate comma as decimal separator | |||
{ | |||
throw CNMRException(NMR_ERROR_INVALIDSTRINGTODOUBLECONVERSION); |
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.
let's also add a test for that, please.
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 would prefer that we do never add such style of code. While it is shorter, it is a landmine and who knows on what situation and platform (or little change by somebody) answer.ptr[0] is evaluated when answer.ptr == nullptr.
Making two if statements out of it is much easier to read and much safer.
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.
if(answer.ptr){
if(answer.ptr[0] == ','){
throw here
}
}
I can make this into two if statements for readability, but that would mean the same. Did you mean anything else?
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.
That's what I meant, yes :)
Allowing leading + sign
Not allowing , separated values
auto answer = fast_float::from_chars(input.data(), input.data()+input.size(), result);
if(answer.ec != std::errc()) {
// check error
}
if(answer.ptr[0] == ',') {
// unexpected delimiter
}