Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Move doesPointTo #2447

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Move doesPointTo #2447

wants to merge 2 commits into from

Conversation

TurkeyMan
Copy link
Contributor

As a bonus on top of #2442, this re-enables the message that complains if you try and move a type with an interior pointer.

I'm not even convinced the assert should exist; if I want to move something, I want to bloody move it!
If I was stupid enough to create a thing with an interior pointer; then shame on me, but that shouldn't cause a compile error when I move it... perhaps the interior pointer is a tiny subset of the thing, which I intend to handle manually. If I need to move it, there's no escape hatch here. It feels like a higher-level concern.

Anyway, aside from the question of whether that assert should even be there at all, this PR pulls ~2000 new lines of code... to emit that error message.
I don't know if I'm comfortable with the idea of introducing 2000 lines to druntime that do nothing except emit an error message when the user made a possible mistake.
I'm not convinced the value justifies the cost...

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @TurkeyMan! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 + druntime#2447"

@thewilsonator
Copy link
Contributor

Is the second commit dependent on the first one? i.e. does the first one need to be there if its already in another PR?

@TurkeyMan
Copy link
Contributor Author

Yes, the first PR should be merged, this one should not be merged... unless someone can convince me otherwise.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Jan 8, 2019

Another thought; opMove, which has been accepted, will make that check incorrect.
It will be the case that with opMove, you will be able to move objects with interior pointers.

@thewilsonator
Copy link
Contributor

Yeah, for compile generated moves you need DIP1014 implemented, but for explicit moves there's no reason why we can't start supporting it now.

@wilzbach
Copy link
Member

Note that without this PR we can't get rid of the now duplicate implementation of move,{Emplace} in Phobos as there are tests for the doesPointTo check.
See also: dlang/phobos#6848

@TurkeyMan
Copy link
Contributor Author

Yeah see, I didn't move that function, because it's obscene. The fact that function is like 2 thousand lines suggests at some serious deficiency somewhere.
Whatever the hell it's for, it should be worked-around ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants