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

[~regression fix] fix Issue 19248 and reopened 18957 #8700

Merged
merged 1 commit into from
Sep 16, 2018

Conversation

9il
Copy link
Member

@9il 9il commented Sep 15, 2018

I am not sure if it is normal regression. The issue 18957 has been fixed, but partially. The example in the issue 18957 is wrong, and it looks like it caused the wrong test.

@9il 9il requested a review from ibuclaw as a code owner September 15, 2018 10:08
@dlang-bot
Copy link
Contributor

dlang-bot commented Sep 15, 2018

Thanks for your pull request, @9il!

Bugzilla references

Auto-close Bugzilla Severity Description
19248 regression Wrong mangle for C++ const STL classes/structs

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8700"

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

No test case and mind the regressions introduced.

@9il
Copy link
Member Author

9il commented Sep 15, 2018

This is not regressions, this is wrong test

@9il
Copy link
Member Author

9il commented Sep 15, 2018

Test fixed now

@9il
Copy link
Member Author

9il commented Sep 15, 2018

BTW, looks like it is further fix for https://issues.dlang.org/show_bug.cgi?id=18957.
Should it be targeted to stable?

@9il 9il changed the title fix Issue 19248 fix Issue 19248 and reopened 18957 Sep 15, 2018
@9il 9il changed the title fix Issue 19248 and reopened 18957 [regression fix] fix Issue 19248 and reopened 18957 Sep 15, 2018
@9il
Copy link
Member Author

9il commented Sep 15, 2018

ping @ibuclaw

@9il 9il changed the title [regression fix] fix Issue 19248 and reopened 18957 [~regression fix] fix Issue 19248 and reopened 18957 Sep 15, 2018
@andralex
Copy link
Member

ping @ibuclaw @WalterBright

@ibuclaw
Copy link
Member

ibuclaw commented Sep 15, 2018

You have not added any tests, only comments.

Copying and pasting the source code of another project is questionable also.

@ibuclaw
Copy link
Member

ibuclaw commented Sep 15, 2018

The test should make sure that linking works with mixed C++ and D. Testing the mangle string is not enough, especially if you claim the old test to be erroneous.

It wasn't erroneous for whoever added the test.

@ibuclaw
Copy link
Member

ibuclaw commented Sep 15, 2018

Should it be targeted to stable?

Stable should only be receiving regressions. It will eventually make its way to master in a week anyway, so up to you.

@9il
Copy link
Member Author

9il commented Sep 15, 2018

Should it be targeted to stable?

Stable should only be receiving regressions. It will eventually make its way to master in a week anyway, so up to you.

OK, lets it be master then

Copying and pasting the source code of another project is questionable also.

removed

The test should make sure that linking works with mixed C++ and D. Testing the mangle string is not enough, especially if you claim the old test to be erroneous.

done

@jacob-carlborg
Copy link
Contributor

Please squash commits when done.

@9il
Copy link
Member Author

9il commented Sep 15, 2018

Please squash commits when done.

rebased

@TurkeyMan
Copy link
Contributor

I'm no linux expert, but I just did some mangling tests with my in-progress C++ work against actual C++ on linux, and reproduced @9il's findings in this PR.
So, LGTM.

@dnadlinger dnadlinger dismissed ibuclaw’s stale review September 16, 2018 11:09

Test case added.

@dlang-bot dlang-bot merged commit 2db16ad into dlang:master Sep 16, 2018
@9il 9il deleted the issue19248 branch September 16, 2018 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants