-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add logic to decipher floating point implicit and explicit casts. #257
base: master
Are you sure you want to change the base?
Conversation
This is unfortunately doign 3 things at once, which is really not idea:
This could go faster if the 2 were separated, as any problem with one wouldn't block the other two. This also doesn't have any integration tests, which is a bit unfortunate. |
6160b08
to
d76cee4
Compare
d76cee4
to
64abded
Compare
a859f2f
to
2204a75
Compare
c27797e
to
d87f793
Compare
d87f793
to
e0b4ec6
Compare
@@ -319,6 +320,12 @@ struct Caster(bool isExplicit, alias bailoutOverride = null) { | |||
return CastKind.IntToBool; | |||
} | |||
|
|||
if (isFloat(bt)) { |
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.
Not really a problem with this patch, but that function could do with a better name. isFloatingPointType
or something like that.
src/d/semantic/caster.d
Outdated
return CastKind.Invalid; | ||
} | ||
|
||
return isSigned(isChar(bt) ? integralOfChar(bt) : bt) |
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 bake int he assumption that the only thing that can convert to integral that isn't an integral is a char. This is the kind of assumption that tend to blow up on one's face later (for instance, we can immagine bool to be convertible to integral, but not being a char).
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 remains incorrect, as isSigned
is not defined for non integral types.
See implementation:
sdc/src/d/common/builtintype.d
Lines 114 to 117 in aa64387
bool isSigned(BuiltinType t) | |
in(isIntegral(t), "isSigned only applys to integral types") { | |
return (t & 0x01) == 0; | |
} |
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 D spec does consider bool to be an integral
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.
Not entirely sure how to proceed
return isFloat(bt) | ||
? CastKind.UnsignedToFloat | ||
: CastKind.Invalid; |
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.
Not a huge deal, but I don't think a ternary is the best approach here. The code has a "if this do that, if this do that, if this do that, if nothing matches then return invalid" kind of structure.
This means you can add/change/remove the previous branches independently without the overall structure of the code changing, which is what you get with the if and then return invalid. With a ternary, the "default" option is commingled with the last pattern you match. Not a blocker, but this is the kind of details that makes for elegant subsequent diffs when things change.
Logic is consistent with dmd with the exception that dmd seems to allow some casts *to* void which are not yet accounted for (if ever) Unit tests have been added.
e0b4ec6
to
d312fee
Compare
f94baa8
to
87a9baf
Compare
cf07b66
to
39f39ba
Compare
4acd1ab
to
e854d3d
Compare
0896895
to
5a79292
Compare
7ce40a5
to
4dbe602
Compare
Logic is consistent with dmd with the exception that dmd seems to allow some casts to void which are not yet accounted for (if ever)
Unit tests have been added.