-
-
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 20909 - .offsetof fails on forward reference of field #11253
Conversation
|
Thanks for your pull request and interest in making D better, @BorisCarvajal! 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 run digger -- build "stable + dmd#11253" |
src/dmd/typesem.d
Outdated
| @@ -3124,6 +3124,7 @@ Expression dotExp(Type mt, Scope* sc, Expression e, Identifier ident, int flag) | |||
| { | |||
| if (ident == Id.offsetof) | |||
| { | |||
| v.dsymbolSemantic(sc); | |||
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 problem here is that sc is the context of the expression, which is not the same as the context of the field. The solution is to use null instead of sc.
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.
Absolutely, thanks.
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.
Done.
|
Any more feedback on this little fix? |
| @@ -3124,6 +3124,7 @@ Expression dotExp(Type mt, Scope* sc, Expression e, Identifier ident, int flag) | |||
| { | |||
| if (ident == Id.offsetof) | |||
| { | |||
| v.dsymbolSemantic(null); | |||
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.
maybe try to surround with v.inuse++ and v.inuse--.
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 replying.
Do you have any recommendations on when to use the inuse variable?
For example in this file I can't find any use of them at dsymbolSemantic() calls.
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.
It's usually only used after someone discover a bug leading to a stack overflow caused by recursive semantic. So my recommendation is only a preventive. I'm really not sure it is useful.
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.
Mutually depedant types can lead to the problem. But mutually dependant types would lead to have the semantic run lazily at from another place, making the bug fixed here impossible to happen...
let's say my suggestion can be ignored.
Run semantic on the variable first to make sure it's an instance field.