-
-
Notifications
You must be signed in to change notification settings - Fork 609
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
Fix Issue 21828 - Enum forward-references just assume int base type #12438
Conversation
|
Thanks for your pull request, @BorisCarvajal! Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12438" |
src/dmd/denum.d
Outdated
| e = e.expressionSemantic(_scope); | ||
| e = resolveProperties(_scope, e); | ||
| e = e.ctfeInterpret(); | ||
| memtype = e.type; |
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.
Couldn't this call getDefaultValue (or a refactored method implementing the primary analysis) instead of partially duplicating it's implementation?
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.
getDefaultValue() calls dsymbolSemantic(this, _scope); we don't need to do that yet.
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.
Hence the hint regarding a refactored method. This copies parts of dsymbolSemantic for EnumMembers while omitting e.g. the type painting, no?
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.
What type painting? The only thing I see I could do is to assign back e ( em.value = e) to prevent evaluating it again in dsymbolsem.d.
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.
E.g. this code will now be skipped in case of a forward reference because memtype is set here?
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.
I think it's better to do the complete semantic pass for now, it's more solid ground. Besides, I've already discovered another bug expanding the test and trying to refactor some code.
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.
Thanks for the review.
|
These kinds of forward-reference issues really need to be tackled systemically. |
|
Added this reduction: E e;
enum E
{
a = "x"
} |
Note: bug name changed