Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxhaton
Copy link
Collaborator

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.

src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
@deadalnix
Copy link
Contributor

This is unfortunately doign 3 things at once, which is really not idea:

  • float to float conversions.
  • int to float.
  • float to int.

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.

src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
@maxhaton maxhaton force-pushed the implicitFloatCasts branch 4 times, most recently from a859f2f to 2204a75 Compare February 26, 2023 18:29
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
@maxhaton maxhaton force-pushed the implicitFloatCasts branch 5 times, most recently from c27797e to d87f793 Compare May 31, 2023 16:47
@maxhaton maxhaton force-pushed the implicitFloatCasts branch from d87f793 to e0b4ec6 Compare June 1, 2023 21:55
@@ -319,6 +320,12 @@ struct Caster(bool isExplicit, alias bailoutOverride = null) {
return CastKind.IntToBool;
}

if (isFloat(bt)) {
Copy link
Contributor

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.

return CastKind.Invalid;
}

return isSigned(isChar(bt) ? integralOfChar(bt) : bt)
Copy link
Contributor

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).

Copy link
Contributor

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:

bool isSigned(BuiltinType t)
in(isIntegral(t), "isSigned only applys to integral types") {
return (t & 0x01) == 0;
}

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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

src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
src/d/semantic/caster.d Outdated Show resolved Hide resolved
Comment on lines +302 to +304
return isFloat(bt)
? CastKind.UnsignedToFloat
: CastKind.Invalid;
Copy link
Contributor

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants