-
-
Notifications
You must be signed in to change notification settings - Fork 599
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 18953 - 32-bit: extern(C++) struct destructor not called cor… #8359
Conversation
Thanks for your pull request, @rainers! 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 "stable + dmd#8359" |
Actually other 32-bit platforms are affected, too. Why do we have to deviate from the C++ ABI to begin with? |
If reviewers could expedite this PR, it should be in 2.081 along with all the other extern(C++) work this cycle. |
@TurkeyMan Knowing that you have reviewed this helps. Please explicitly mark it "Approved" if that's your determination. |
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 don't know much about the C++ ABI incompatibility on 32-bit platforms, but this change just lifts the restriction for 64-bit Windows and the test and attribute inference LGTM too.
@@ -1081,6 +1083,7 @@ DtorDeclaration buildExternDDtor(AggregateDeclaration ad, Scope* sc) | |||
ad.members.push(func); | |||
func.addMember(sc2, ad); | |||
func.dsymbolSemantic(sc2); | |||
func.functionSemantic(); // to infer attributes |
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.
While this inference change makes sense, I'm not sure whether it's a good idea to combine it with a PR that "just adds a test".
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 commit message already said otherwise, I've updated the PR description now, too.
Does it have to be rebased to stable to make it into 2.081? |
Yup. |
Rebased to stable. |
…through runtime Win32 already fixed by d08e0fb other 32-bit platforms have the same issue
dlang/dmd#8359 introduces direct call expressions for struct methods (which cannot be virtual anyway and are so always called directly). Our previous code relied on the instance being a class. There are multiple ways to fix this; I went with this one.
…rectly through runtime
mostly fixed by d08e0fb, but 32-bit platforms other than Win32 are affected, too. Also added function attribute inference for generated shim function.