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

Fix Issue 19838 - RefCounted fails to instantiate due to pureness of … #7006

Merged
merged 1 commit into from
Jul 10, 2019

Conversation

marcioapm
Copy link
Contributor

…moveEmplace

@dlang-bot
Copy link
Contributor

dlang-bot commented May 15, 2019

Thanks for your pull request and interest in making D better, @marcioapm! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Auto-close Bugzilla Severity Description
19838 enhancement RefCounted fails to instantiate due to pureness of moveEmplace

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 "stable + phobos#7006"

@wilzbach
Copy link
Member

Please add a test case to avoid future regressions.

@marcioapm
Copy link
Contributor Author

marcioapm commented May 15, 2019

@wilzbach I can't find a reduced test case that reproduces the behavior on our codebase.

Somehow it seems that inferring pureness doesn't work in this particular case, which points to a deeper issue.

This PR will alleviate the situation introduced by manually annotating RefCountedStore.move() as pure in c4324f4#diff-4e008aedb3026d4a84f58323e53bf017

@atilaneves
Copy link
Contributor

Shouldn't this be a dmd bug?

@marcioapm
Copy link
Contributor Author

Ping! Any comments on this and if this, or a better fix are going to be merged?

@thewilsonator
Copy link
Contributor

Please add the test case for the issue.

@marcioapm
Copy link
Contributor Author

marcioapm commented Jul 10, 2019

I did try to find a reduced test case but wasn't able to. I suppose once the real bug is found and understood better it will be easier.

Despite not being a real fix, it will at least hopefully revert things to how they used to be before the Phobos change was introduced.

This bug is preventing us from upgrading without a big code refactor so we are stuck in 2.083.1

@thewilsonator
Copy link
Contributor

OK.

@thewilsonator
Copy link
Contributor

Also please target stable.

@marcioapm marcioapm changed the base branch from master to stable July 10, 2019 09:56
@marcioapm
Copy link
Contributor Author

Done - Thanks!

@dlang-bot dlang-bot merged commit 1a66781 into dlang:stable Jul 10, 2019
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.

5 participants