Skip to content

Add logic to decipher floating point implicit and explicit casts.#257

Open
maxhaton wants to merge 1 commit intosnazzy-d:masterfrom
maxhaton:implicitFloatCasts
Open

Add logic to decipher floating point implicit and explicit casts.#257
maxhaton wants to merge 1 commit intosnazzy-d:masterfrom
maxhaton:implicitFloatCasts

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.

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

@maxhaton maxhaton force-pushed the implicitFloatCasts branch 4 times, most recently from a859f2f to 2204a75 Compare February 26, 2023 18:29
@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
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

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