-
Notifications
You must be signed in to change notification settings - Fork 1
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
(interpreter) Fix integer expansions to bigint
#234
Conversation
// TODO: Might need to revisit this to support more types as we implement #70. | ||
_ => throw new IllegalStateException($"Unsupported conversion from {value.GetType().ToTypeKeyword()} to {targetTypeReference.ClrType.ToTypeKeyword()}") | ||
}; | ||
} |
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 is the essence of the fix.
Also, the TODO comment for #70 is quite important. The easiest way to ensure that it gets fixed is probably to add more test cases, so that we properly test this with values of all types expected to be supported (i.e. expanding this list as we support more types, uint
, ulong
and potentially decimal
- there are BigInteger
constructors for all of these that could be used).
public void exponential_integer_literals_as_variable_initializer() | ||
[Theory] | ||
[InlineData("2", "12", "4096")] | ||
[InlineData("10", "3.5", "3162")] // TODO: Fix this once the `dynamic` PR gets merged. This is incorrectly coerced to BigInteger in the current implementation. |
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.
🙈 The correct value here would be something like 3162.2776601683795
.
bigint
bigint
double
expansions to bigint
double
expansions to bigint
bigint
For reference, the semantics after this MR got merged was insane:
(Using a This was corrected in #237:
|
While working on #225, I discovered that some semantics here was essentially broken. The exponential operator only worked properly with
int
+int
orbigint
+int
operands. When one of the operands was afloat
, it was impossible to assign the result to a variable.