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

Compiler: Do not cast enum value to i32 if not specified. Fixes #194 #2703

Merged
merged 1 commit into from Jun 1, 2016

Conversation

Projects
None yet
3 participants
@fernandes
Contributor

fernandes commented Jun 1, 2016

If enum base type is not specified, default is assumed (i32), and if any
enum value is specified with a different type it should be casted to
i32, but it's an undesired behavior, thats why now an exception is
raised to alert developer.

@fernandes

This comment has been minimized.

Contributor

fernandes commented Jun 1, 2016

handle #194

@lbguilherme

This comment has been minimized.

Contributor

lbguilherme commented Jun 1, 2016

The error message seems misleading: enum value type must be an integer type. u32 is a integer type

@asterite

This comment has been minimized.

Contributor

asterite commented Jun 1, 2016

Agree with @lbguilherme, maybe say "enum value must be an Int32"

@fernandes

This comment has been minimized.

Contributor

fernandes commented Jun 1, 2016

you're totally right, updated the PR with the improved exception message.. thank you @lbguilherme and @asterite

@fernandes

This comment has been minimized.

Contributor

fernandes commented Jun 1, 2016

@asterite build passed in case you want to merge it 👍

if default_value.is_a?(Crystal::NumberLiteral)
enum_base_kind = enum_base_type.kind
node_kind = return_enum_kind(default_value)
if (enum_base_kind == :i32) && (enum_base_kind != node_kind)

This comment has been minimized.

@asterite

asterite Jun 1, 2016

Contributor

How about we remove return_enum_kind and just use default_value.kind here?

This comment has been minimized.

@fernandes

fernandes Jun 1, 2016

Contributor

I tried doing this, but at compile time, the compiler expects default_value as a Crystal::ASTNode and #kind is specific to Crystal::NumberLiteral, so can't call it here,

I created return_enum_kind with signature expecting (node : NumberLiteral) then I can call #kind inside.

I don't know if I can handle another way, if you can point me to a solution I change immediately

This comment has been minimized.

@asterite

asterite Jun 1, 2016

Contributor

I just tried the change it and it works fine. The compiler knows default_value is a NumberLiteral because it's inside if default_value.is_a?(Crystal::NumberLiteral). Maybe you didn't have that before?

In any case, doing it with a return_enum_kind should also give an error because you are not providing overloads for all ASTNode types (which means that the compiler knows that default_value can only be a NumberLiteral).

This comment has been minimized.

@fernandes

fernandes Jun 1, 2016

Contributor

maybe was my mistake... now I run the specs and it's passing, let me rebuild the compiler and run all spec to make sure build won't break, I'm pushing in a few minutes 👍

@asterite

This comment has been minimized.

Contributor

asterite commented Jun 1, 2016

@fernandes Thank you!

🙇 <- for touching compiler code :-)

I just made a comment. After that, I'll merge.

@fernandes

This comment has been minimized.

Contributor

fernandes commented Jun 1, 2016

@asterite my pleasure 🙇

Compiler: Do not cast enum value to i32 if not specified. Fixes #194
If enum base type is not specified, default is assumed (i32), and if any
enum value is specified with a different type it should be casted to
i32, but it's an undesired behavior, thats why now an exception is
raised to alert developer.
@fernandes

This comment has been minimized.

Contributor

fernandes commented Jun 1, 2016

@asterite I think now everything is fine :)

@asterite

This comment has been minimized.

Contributor

asterite commented Jun 1, 2016

Perfect, thank you! ❤️

@asterite asterite merged commit 635b96f into crystal-lang:master Jun 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment