-
Notifications
You must be signed in to change notification settings - Fork 299
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
Plus, minus and times crossing 32 to 64 bit boundary #488
base: master
Are you sure you want to change the base?
Conversation
…ithin 64 bit integer range. Fixes #483
Codecov Report
@@ Coverage Diff @@
## master #488 +/- ##
==========================================
- Coverage 87.53% 87.28% -0.26%
==========================================
Files 69 69
Lines 2688 2697 +9
Branches 639 639
==========================================
+ Hits 2353 2354 +1
- Misses 212 213 +1
- Partials 123 130 +7
Continue to review full report at Codecov.
|
{ | ||
TestPlusNumericBoundaryByContext(_contextV20); | ||
TestPlusNumericBoundaryByContext(_contextV21); | ||
Helper.AssertTemplateResult(expected: "2147483648", template: "{{ '2147483647' | plus: 1 }}", syntax: _contextV21.SyntaxCompatibilityLevel); |
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.
Should we comment that this tests a string which is supported in Syntax 2.1? Should we also test a string as a local variable?
catch (OverflowException) when (input is int) | ||
{ | ||
return DoMathsOperation(context, Convert.ToInt64(input), operand, Expression.MultiplyChecked); | ||
} | ||
} |
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.
what about divide by fraction? e.g. 2147483647 / 0.5 might result in an overflow.
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.
Should convert to a floating point hence range is different.
? string.Concat(input, operand) | ||
: DoMathsOperation(context, input, operand, Expression.AddChecked); | ||
} | ||
catch (OverflowException) when (input is int) |
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.
wouldn't uint also have a boundary? and smaller integer values, like short? Additionally, Ruby implementation supports adding 1 to decimal max without losing precision.
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 tried bytes and short, they worked fine without modification, I can add a few tests to demonstrate it. I didn't try uint.
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.
BigInteger is out of scope for a bug fix, that support would be a feature in itself.
syntax: context.SyntaxCompatibilityLevel); | ||
|
||
// NOTE(David Burg): We don't have support for BigInteger as this time. | ||
Helper.AssertTemplateResult( |
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.
todo: also add tests demonstrating that byte max, short max work already, but double range limit cannot be extended beyond current overflow.
Handle integer numbers beyond the range of 32 bit integer but still within 64 bit integer range.
Fixes #483