-
-
Notifications
You must be signed in to change notification settings - Fork 668
Fix Issue 18578 - First enum value assigned 0 instead of EnumBaseType.init #8090
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
Conversation
|
Thanks for your pull request and interest in making D better, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8090" |
|
Also this: #7996 |
| } | ||
| Expression e = new IntegerExp(em.loc, 0, Type.tint32); | ||
| e = e.implicitCastTo(sc, t); | ||
| Expression e = new IntegerExp(em.loc, 0, t); |
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 too familiar with the AST, but @LemonBoy's change "looks" more correct:
Expression e = t.defaultInitLiteral(em.loc);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.
That was my first option also, but for some reason it led to segfaults when running the tests. From my point of view, enums are an enumeration of constants that are represented by integers behind the scenes, so this solution looks correct (notice that the cast is not needed anymore)
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 trouble is there is significant code depending on it being 0. A deprecation cycle will be needed. @LemonBoy 's solution is more correct, but your test case is better. Both of you fixing the same bug is a pity, as now I have to close one. :-(
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.
Yeah merging this one first seems reasonable.
@RazvanN7 The language spec doesn't say anything about enums being "integers behind the scenes" (https://dlang.org/spec/enum.html). In fact, if enums had this restriction you couldn't do things like this:
enum message = "MyMessage"; // not an integer behind the scenes|
Better yet, I'll just approve this one, and regard @LemonBoy 's fix as an enhancement. |
Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.
Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.
Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.
Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.
Let's deprecate this behaviour that, according to the specification, is incorrect. Builds up on dlang#8090.
No description provided.