Skip to content
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

mangle C++ ::std and ::std::allocator correctly #3873

Merged
merged 1 commit into from
Aug 22, 2014

Conversation

WalterBright
Copy link
Member

On Linux, there are shortcut mangles for ::std and ::std::allocator which were not implemented.

Not done on Windows because C++ template mangling currently isn't done at all on Windows. Need to fix that with another PR.

Also wound up removing some tabs from the test files.

@WalterBright
Copy link
Member Author

This should add enough to enable attempts to interface with std::vector from D.

@yebblies
Copy link
Member

Not done on Windows because C++ template mangling currently isn't done at all on Windows.

What? Mangling templates was the main motivation for moving windows C++ mangling into the frontend. eg Array<Expression*> is mangled correctly.

@WalterBright
Copy link
Member Author

@yebblies I was wrong, but the example still isn't mangling right. Will need more investigation.

@WalterBright
Copy link
Member Author

The issue with mangling for Win32 is entirely different, so shouldn't be the target of this PR. This one is ready to go.

@@ -1,279 +1,287 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've screwed up the line endings in here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@yebblies
Copy link
Member

The issue with mangling for Win32 is entirely different, so shouldn't be the target of this PR. This one is ready to go.

Just declare your own versions of vector/allocator instead of #includeing vector.

@WalterBright
Copy link
Member Author

Just declare your own versions of vector / allocator instead of #include ing vector.

I don't see how we can expect users to do that.

{
return true;
}
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant control flow - just use

if (exist(p->isTemplateInstance()->tempdecl))
{
    return true;
}
p = p->toParent();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p->isTemplateInstance() may return null, so it is not equivalent to your snippet.

@andralex
Copy link
Member

This is awesome and lgtm modulo nits. @yebblies have your concerns been addressed?

@yebblies
Copy link
Member

Just declare your own versions of vector / allocator instead of #include ing vector.

I don't see how we can expect users to do that.

Who said anything about users doing that? I'm saying do it in the test case, so this will pass on all platforms. One that note, why is this linux-only? Why no OSX?

This is awesome and lgtm modulo nits. @yebblies have your concerns been addressed?

@andralex No, they haven't.

@WalterBright
Copy link
Member Author

Who said anything about users doing that?

It was unclear what you were suggesting.

I'm saying do it in the test case, so this will pass on all platforms.

Since users will be using # include <vector>, that's what the test should be exercising, not our own custom version of vector.

Why no OSX?

OSX and FreeBSD added.

@yebblies
Copy link
Member

Since users will be using # include <vector>, that's what the test should be exercising, not our own custom version of vector.

No, this test is about mangling, and it should just exercise the mangling. The D side certainly doesn't test anything else. If you want to add a test that actually uses vector I would have no problem with it, but let's not pretend that's what this one does.

OSX and FreeBSD added.

You only added them to the C++ side.

@WalterBright
Copy link
Member Author

No, this test is about mangling, and it should just exercise the mangling. The D side certainly doesn't test anything else. If you want to add a test that actually uses vector I would have no problem with it, but let's not pretend that's what this one does.

I implemented your other suggestions, thanks, but this one I disagree with. The idea is to test the mangling with the mangling in <vector>, as it is supposed to match that, not our recreation of it. If <vector> changes, I certainly wish to be informed about it via the test failing, instead of the test continuing to work.

Also, <vector> can be presumed to exist on any system one is trying to link with C++, just as <stdio.h> would be. There's no downside to referencing it. Creating our own version of <vector> just to avoid # include'ing it is pointless extra work, and extra work for anyone maintaining it because he's now got to go through the C++ system .h files to verify that it matches.

@yebblies
Copy link
Member

I implemented your other suggestions, thanks,

Thankyou.

but this one I disagree with. The idea is to test the mangling with the mangling in <vector>, as it is supposed to match that, not our recreation of it. If <vector> changes, I certainly wish to be informed about it via the test failing, instead of the test continuing to work.

If it was a test that actually used vector, then I would agree. But it's not, it just checks that the special mangling rules are implemented correctly. Just look at the definitions in cppa.d, you're already recreating vector and assuming it will match.

Also, can be presumed to exist on any system one is trying to link with C++, just as <stdio.h> would be. There's no downside to referencing it.

As I'm sure you know, dmc does not come with stl installed by default. Or are you not planning to support this on win32?

Creating our own version of just to avoid # include'ing it is pointless extra work,

It's trivial extra work, which you already did on the D side.

and extra work for anyone maintaining it because he's now got to go through the C++ system .h files to verify that it matches.

I find it hard to believe that writing the C++ equivalent of

extern (C++, std)
{
    struct vector(T, A = allocator!T) { }

    struct allocator(T) { }
}

is a significant burden.

@yebblies
Copy link
Member

Anyway, I'll merge it because my only objection is the test case and it's still better than no tests.

@yebblies
Copy link
Member

Auto-merge toggled on

@WalterBright
Copy link
Member Author

thank you

@yebblies
Copy link
Member

The dmc STL is a separate download, and is still free
I should probably just merge it in, the reasons for it being separate no longer make any sense

That would be good. I would have not have this objection if it was merged.

This feature does not yet work with dmc, that's why it is not enabled for it, so the point is currently moot

That's the whole point, it could work right now if you followed my suggestion instead of including vector.

The test case tests that it matches as far as name mangling goes. Your suggestion will NOT test whether it matches , and adds maintenance and other extra work for an inferior test. I do not understand your objection to using .

This sounds similar to my argument about the elf headers. Maybe I am getting through after all!

@WalterBright
Copy link
Member Author

That's the whole point, it could work right now if you followed my suggestion instead of including vector.

The test case would work, but the user still could not connect to std::vector.

@yebblies
Copy link
Member

The test case would work, but the user still could not connect to std::vector.

Yes...? With your patch neither works.

yebblies added a commit that referenced this pull request Aug 22, 2014
mangle C++ ::std and ::std::allocator correctly
@yebblies yebblies merged commit 7f28fa6 into dlang:master Aug 22, 2014
@WalterBright WalterBright deleted the std-mangling branch August 22, 2014 17:44
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.

3 participants