Moved `move`, `moveEmplace`, and `forward` to `core.lifetime` #2442

TurkeyMan · 2019-01-05T04:20:14Z

WIP, I still need to wrangle phobos dependencies...

dlang-bot · 2019-01-05T04:20:15Z

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#2442"

thewilsonator · 2019-01-05T06:22:06Z

src/core/lifetime.d(307): Deprecation: module `core.internal.traits` member `maxAlignment` is not visible from module `lifetime`
src/core/lifetime.d(1323): Error: template instance `AliasSeq!(fwd, forward!(args[1 .. __dollar]))` template `AliasSeq` is not defined
src/core/lifetime.d(1358): Error: template instance `core.lifetime.__unittest_L1353_C7.bar!(int, string).bar.forward!(_param_0, _param_1)` error instantiating

Looks like you need to explicitly import core.interal.traits.

TurkeyMan · 2019-01-05T08:17:30Z

Yup, is WIP. Just here so people can complain eagerly ;)

PetarKirov · 2019-01-05T08:27:51Z

There'll be nothing to complain when the build is fixed ;) Thanks for taking the initiative!

TurkeyMan · 2019-01-05T09:10:19Z

WTF is with Phobos!! The dependence hierarchies are all completely insane!
This is basically impossible, the dependency graph is just wild... move pulls the whole universe!
Check out C++'s std::move, it should be that simple!

TurkeyMan · 2019-01-05T09:45:47Z

I'm not sure what the go with this error is...
I cut and pasted the code from phobos verbatim, and this test works there :/

wilzbach · 2019-01-05T10:35:38Z

CI says:

src/core/lifetime.d(1904): Error: struct `core.lifetime.__unittest_L1934_C7.NoCopy` is not copyable because it is annotated with `@disable`
src/core/lifetime.d(1715): Error: template instance `core.lifetime.moveEmplace!(NoCopy[2])` error instantiating
src/core/lifetime.d(1709):        instantiated from here: `moveImpl!(NoCopy[2])`
src/core/lifetime.d(1667):        instantiated from here: `trustedMoveImpl!(NoCopy[2])`
src/core/lifetime.d(1948):        instantiated from here: `move!(NoCopy[2])`
src/core/lifetime.d(1904): Error: struct `core.lifetime.__unittest_L1934_C7.NoCopy` is not copyable because it is annotated with `@disable`
src/core/lifetime.d(1715): Error: template instance `core.lifetime.moveEmplace!(NoCopy[2])` error instantiating
src/core/lifetime.d(1709):        instantiated from here: `moveImpl!(NoCopy[2])`
src/core/lifetime.d(1667):        instantiated from here: `trustedMoveImpl!(NoCopy[2])`
src/core/lifetime.d(1948):        instantiated from here: `move!(NoCopy[2])`

Does

make -f posix.mak unittest-debug

work for your locally?

thewilsonator · 2019-01-05T12:00:13Z

It fails because you haven't copied all of moveEmplace, missing is:

    else static if (isStaticArray!T)
    {
        for (size_t i = 0; i < source.length; ++i)
            move(source[i], target[i]);
    }

TurkeyMan · 2019-01-05T19:56:51Z

Oh.... shit.
I wasn't copying from phobos HEAD, I accidentally pressed "go to definition" in my editor, and it opened the version of the file from my local DMD import path from my local compiler install, which was from the last release >_<

I hope I haven't done that elsewhere!! O_O
I need to audit everything I've done!

thewilsonator

LGTM

TurkeyMan · 2019-01-07T20:24:10Z

Is this G2G? That one test fail looks completely unrelated...

wilzbach · 2019-01-07T22:03:24Z

Is this G2G? That one test fail looks completely unrelated...

Yup. Deprecated the build for you.

TurkeyMan · 2019-01-07T22:18:59Z

What does that mean? Can this merge?

wilzbach · 2019-01-07T22:27:32Z

src/core/lifetime.d

+//    static if (!is(T == class) && hasAliasing!T) if (!__ctfe)
+//    {
+//        import std.exception : doesPointTo;
+//        assert(!doesPointTo(source, source), "Cannot move object with internal pointer.");


How do we ensure we fix this before the next release?

We can't ensure that; but I'm starting to look into core.traits, which will lead to the solution.
We are paying an obscenely high compile-time cost for this error message, and it's not even that great!

wilzbach · 2019-01-07T22:44:13Z

How do we ensure we fix this before the next release?

Okay, I ported doesPointTo for you to druntime. Feel free to move it to core.internal or wherever it belongs. I didn't port hasAliasing as it pulled in too much for now.

TurkeyMan · 2019-01-07T23:28:46Z

I'm not sure your patch is right... you're missing the hasAliasing guard, which will make the assert more inclusive... false positives?

But if you wanna keep it, can you move doesPointTo to core.internal.traits? That's the appropriate dumping ground. I don't want core.lifetime to already be a dumping ground at the moment of its birth...

wilzbach · 2019-01-08T00:57:11Z

I'm not sure your patch is right... you're missing the hasAliasing guard, which will make the assert more inclusive... false positives?

I'm aware and I wanted to port hasAliasing too. I just ran out of time.

But if you wanna keep it, can you move doesPointTo to core.internal.traits? That's the appropriate dumping ground. I don't want core.lifetime to already be a dumping ground at the moment of its birth...

Yeah I know. Just wanted to quickly test whether it works on all CIs and didn't have much time.

wilzbach · 2019-01-08T00:58:38Z

That's the appropriate dumping ground

So yeah I managed to port hasAliasing and doesPointTo. It's a bit of rabbit hole though:

 2 files changed, 1922 insertions(+), 6 deletions(-)

However, of course we can remove all these lines from Phobos and replace them with aliases.

TurkeyMan · 2019-01-08T01:23:29Z

We can't remove them from phobos unless we put then in a public module, with doco generated and tests... then we can make phobos aliases.

TurkeyMan · 2019-01-08T01:24:58Z

See... look at that patch now... look how much code you pulled.
I got here too, and then I came to my senses, concluded that it was completely unreasonable to draw on that much code to emit an error message, and commented out the line.
I personally think the line should remain commented out ;)

I don't think the error message is reasonable with respect to the volume of supporting code.
The error's just not even that great!

TurkeyMan · 2019-01-08T01:29:47Z

I'm not comfortable to pull this. Can you make your amendment a separate PR on top of my original one, and then we can discuss the best approach and where to draw the lines without blocking my original PR?

wilzbach · 2019-01-08T02:19:48Z

We can't remove them from phobos unless we put then in a public module, with doco generated and tests... then we can make phobos aliases.

Why shouldn't we be able to alias them? Even DMD's documentation generator supports exposing aliased symbols. We would just need to keep the public tests in Phobos.

wilzbach · 2019-01-08T02:21:01Z

concluded that it was completely unreasonable to draw on that much code

Not sure as we'll probably end up pulling most of this code from druntime at some point anyhow.

TurkeyMan · 2019-01-08T02:27:44Z

Why shouldn't we be able to alias them?

I noticed while moving some other stuff, that the phobos stuff is not necessarily in sync with core.internal... a full audit needs to happen.
Phobos has received development, and core.internal is wherever it was when it was copied.

Any effort to create core.traits should just delete core.internal.traits, and move directly from phobos one symbol at a time.

Not sure as we'll probably end up pulling most of this code from druntime at some point anyhow.

Right, but that's a carefully considered process.

Anyway, I made a new PR where we can discuss that pickle.

TurkeyMan · 2019-01-08T02:29:05Z

I also noticed some code in core.internal.traits which was simplified compared to the phobos source material, because a subset of functionality wasn't used by whatever caused it to be pulled in, and it minimised dependencies for unused paths.

thewilsonator · 2019-01-08T06:44:36Z

Auto-merge toggled on

wilzbach · 2019-01-31T15:05:09Z

FYI I started to remove the now duplicate implementations in Phobos:

But we can't move emplace yet as we need a public version of emplaceRef. Though imho we should simply set it public as emplaceRef is used all over in Phobos (see e.g. https://github.com/dlang/phobos/search?q=emplaceRef&unscoped_q=emplaceRef).
It's also part of the proposed RCArray PR for druntime (#2348).

CC @edi33416

andralex · 2019-02-12T16:56:11Z

We can for now make emplaceRef public and docummented. @wilzbach would that work?

wilzbach · 2019-02-12T16:57:56Z

We can for now make emplaceRef public and docummented. @wilzbach would that work?

Yup, that's basically what I proposed ;-)

andralex · 2019-02-12T17:03:37Z

Another possibility is to overload, where are the ambiguities?

TurkeyMan requested review from andralex and wilzbach as code owners January 5, 2019 04:20

TurkeyMan force-pushed the move_forward branch from 361999c to cf5fe64 Compare January 5, 2019 04:30

TurkeyMan force-pushed the move_forward branch from cf5fe64 to 485cca5 Compare January 5, 2019 08:43

TurkeyMan requested review from MartinNowak and schveiguy as code owners January 5, 2019 08:43

TurkeyMan force-pushed the move_forward branch from 485cca5 to 4a19e57 Compare January 5, 2019 08:51

TurkeyMan force-pushed the move_forward branch from 4a19e57 to 6ce26fb Compare January 5, 2019 09:14

TurkeyMan force-pushed the move_forward branch 2 times, most recently from 6bc63f1 to 37eb380 Compare January 5, 2019 20:13

Moved move and forward to core.lifetime.

27b41a0

TurkeyMan force-pushed the move_forward branch from 37eb380 to 27b41a0 Compare January 5, 2019 20:22

thewilsonator approved these changes Jan 5, 2019

View reviewed changes

wilzbach approved these changes Jan 7, 2019

View reviewed changes

wilzbach force-pushed the move_forward branch from 6a9dc60 to 00e2601 Compare January 8, 2019 00:56

wilzbach force-pushed the move_forward branch 2 times, most recently from c312ede to 5df245f Compare January 8, 2019 01:02

TurkeyMan force-pushed the move_forward branch from 5df245f to 27b41a0 Compare January 8, 2019 02:08

TurkeyMan mentioned this pull request Jan 8, 2019

Move doesPointTo #2447

Open

thewilsonator merged commit 24cd5d7 into dlang:master Jan 8, 2019

TurkeyMan deleted the move_forward branch January 8, 2019 08:08

This was referenced Jan 31, 2019

Use moved core.lifetime.forward for std.functional.forward dlang/phobos#6847

Merged

Use moved core.lifetime.move{,Emplace} for std.algorithm.mutation.move{,Emplace} dlang/phobos#6848

Open

edi33416 mentioned this pull request Feb 27, 2019

Move emplaceRef to core/internal #2498

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved `move`, `moveEmplace`, and `forward` to `core.lifetime` #2442

Moved `move`, `moveEmplace`, and `forward` to `core.lifetime` #2442

TurkeyMan commented Jan 5, 2019 •

edited

Loading

dlang-bot commented Jan 5, 2019

thewilsonator commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

PetarKirov commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

wilzbach commented Jan 5, 2019

thewilsonator commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

thewilsonator left a comment

TurkeyMan commented Jan 7, 2019

wilzbach commented Jan 7, 2019

TurkeyMan commented Jan 7, 2019

wilzbach Jan 7, 2019

TurkeyMan Jan 7, 2019

wilzbach commented Jan 7, 2019

TurkeyMan commented Jan 7, 2019

wilzbach commented Jan 8, 2019

wilzbach commented Jan 8, 2019 •

edited

Loading

TurkeyMan commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019 •

edited

Loading

TurkeyMan commented Jan 8, 2019

wilzbach commented Jan 8, 2019

wilzbach commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019

thewilsonator commented Jan 8, 2019

wilzbach commented Jan 31, 2019 •

edited

Loading

andralex commented Feb 12, 2019

wilzbach commented Feb 12, 2019

andralex commented Feb 12, 2019

Moved move, moveEmplace, and forward to core.lifetime #2442

Moved move, moveEmplace, and forward to core.lifetime #2442

Conversation

TurkeyMan commented Jan 5, 2019 • edited Loading

dlang-bot commented Jan 5, 2019

Bugzilla references

Testing this PR locally

thewilsonator commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

PetarKirov commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

wilzbach commented Jan 5, 2019

thewilsonator commented Jan 5, 2019

TurkeyMan commented Jan 5, 2019

thewilsonator left a comment

Choose a reason for hiding this comment

TurkeyMan commented Jan 7, 2019

wilzbach commented Jan 7, 2019

TurkeyMan commented Jan 7, 2019

wilzbach Jan 7, 2019

Choose a reason for hiding this comment

TurkeyMan Jan 7, 2019

Choose a reason for hiding this comment

wilzbach commented Jan 7, 2019

TurkeyMan commented Jan 7, 2019

wilzbach commented Jan 8, 2019

wilzbach commented Jan 8, 2019 • edited Loading

TurkeyMan commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019 • edited Loading

TurkeyMan commented Jan 8, 2019

wilzbach commented Jan 8, 2019

wilzbach commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019

TurkeyMan commented Jan 8, 2019

thewilsonator commented Jan 8, 2019

wilzbach commented Jan 31, 2019 • edited Loading

andralex commented Feb 12, 2019

wilzbach commented Feb 12, 2019

andralex commented Feb 12, 2019

Moved `move`, `moveEmplace`, and `forward` to `core.lifetime` #2442

Moved `move`, `moveEmplace`, and `forward` to `core.lifetime` #2442

TurkeyMan commented Jan 5, 2019 •

edited

Loading

wilzbach commented Jan 8, 2019 •

edited

Loading

TurkeyMan commented Jan 8, 2019 •

edited

Loading

wilzbach commented Jan 31, 2019 •

edited

Loading