Skip to content

Fix C++ declarations causing link errors#1421

Closed
rainers wants to merge 1 commit intoldc-developers:merge-2.071from
rainers:fix_linkage
Closed

Fix C++ declarations causing link errors#1421
rainers wants to merge 1 commit intoldc-developers:merge-2.071from
rainers:fix_linkage

Conversation

@rainers
Copy link
Contributor

@rainers rainers commented Apr 10, 2016

These are the symbols that caused actual link errors on Windows, but there are actually a lot more places in the headers that are wrong (i.e. adding const to an override does not override in C++, but creates a new vtable slot (if virtual)). This should be fixed in general upstream.

@JohanEngelen
Copy link
Member

( It'd be so nice if someone would write a C++-D interop checker tool, to check that the C++ headers and D declarations are the same. :) As a start, just a tool to check class and vtable sizes would catch these tricky to find errors. )

@rainers
Copy link
Contributor Author

rainers commented Apr 10, 2016

As a start, just a tool to check class and vtable sizes would catch these tricky to find errors.

This would not find the issue in this PR as it introduced a non-virtual member function.
IIRC Daniel thought about generating the C++ headers from D code. That could help, too.

@JohanEngelen
Copy link
Member

but creates a new vtable slot (if virtual)

Was commenting on that.

@yebblies
Copy link

IIRC Daniel thought about generating the C++ headers from D code. That could help, too.

It pretty much works: dlang/dmd#5082

@rainers
Copy link
Contributor Author

rainers commented Apr 10, 2016

It pretty much works: dlang/dmd#5082

Looks pretty good. From reading funcToBuffer, it seems const modifiers on member functions are not emitted, so overrides are ok, but mangling is not.
How far do you think it is from being used? Does it make sense to still fix the headers regarding const overrides?

@rainers
Copy link
Contributor Author

rainers commented Apr 16, 2016

I created a PR for dmd: dlang/dmd#5673

@rainers
Copy link
Contributor Author

rainers commented Apr 17, 2016

I created a PR for dmd: dlang/dmd#5673

This has been merged. Is it ok to cherry-pick this into the merge-2.071 branch?

@JohanEngelen
Copy link
Member

JohanEngelen commented Apr 17, 2016

Is it ok to cherry-pick this into the merge-2.071 branch?

Yes!
Nice work on getting merge-2.071 closer to green.

@dnadlinger
Copy link
Member

dnadlinger commented Apr 17, 2016

Yes, of course it is. Feel free to play it "fast and loose" with the merge-… branches until they are a bit closer to green. It would be great if you could include the DMD commit hash (and maybe the PR number) in the commit message for future reference, though. Just directly push the commit once you've amended the message.

@dnadlinger dnadlinger closed this Apr 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants