NotificationsYou must be signed in to change notification settings
Fork729
Star1.2k

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

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

Open

wilzbach wants to merge1 commit intodlang:master

base:master

Choose a base branch

fromwilzbach:move

Open

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

wilzbach wants to merge1 commit intodlang:masterfromwilzbach:move

+19 −312

Conversation

Copy link

Member

wilzbach commentedJan 31, 2019

Indlang/druntime#2442,doesPointTo wasn't ported as it's quite big (dlang/druntime#2447).

That's why one test which test this behavior would fail now. Should we simply movedoesPointTo to druntime as proposed, s.t. we don't need to maintain two implementations?

Use moved core.lifetime.move{,Emplace} for std.algorithm.mutation.mov…

a5b7c50

…e{,Emplace}

Copy link

Contributor

dlang-bot commentedJan 31, 2019

Thanks for your pull request,@wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, pleasereference a Bugzilla issue or create amanual changelog.

Testing this PR locally

If you don't have alocal development environment setup, you can useDigger to test this PR:

dub fetch diggerdub run digger -- build"master + phobos#6848"

wilzbach requested review fromandralex,JackStouffer andPetarKirov ascode owners

January 31, 2019 15:00

This was referencedJan 31, 2019

Move doesPointTodlang/druntime#2447

Open

Movedmove,moveEmplace, andforward tocore.lifetimedlang/druntime#2442

Merged

edi33416 approved these changes

Feb 20, 2019

View reviewed changes

edi33416 reviewed

Feb 20, 2019

View reviewed changes

std/algorithm/mutation.d

		assertThrown!Error(move(p));
		assertThrown!Error(move(p, pp));
		assertThrown!Error(swap(p, pp));
		//import std.exception : assertThrown;

Copy link

Contributor

Choose a reason for hiding this comment

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

Why not delete instead of commenting?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I wanted to see whether this is the only issue.
As explained, the core.lifetime functions are technically different and using them is a breaking change.

wilzbach mentioned this pull request

Oct 21, 2020

Help compiler deducepure for moveEmplaceImpl#7670

Closed

dlang-bot added Merge:Needs Rebase Merge:stalled labels

May 27, 2021

LightBender added the Review:SalvageThis PR needs work, but we want to keep it and it can be salvaged. label

Oct 27, 2024

Copy link

Contributor

LightBender commentedOct 27, 2024

@thewilsonator this one is for you.

Copy link

Member

0xEAB commentedMar 14, 2025

This one is painful becausecore.lifetime didn’t do its homework and breaks compatibility with existing code.
See:https://github.com/dlang/dmd/blob/35f1146a1b537e9a361fa28098a1cfc9a384178f/druntime/src/core/lifetime.d#L2192

Which is particularly painful to fix/emulate downstream because of theif (false) { nonTrustedImpl(); } trustedImpl();@safety hacks used in the code that are inaccessibly from the outside (private “impl” functions).

Copy link

Member

0xEAB commentedMar 14, 2025•
edited
Loading

The code forces@system at users of the public API, yet internally relies on having the chance of having@safe inferred.

Copy link

Member

0xEAB commentedMar 14, 2025•
edited
Loading

std/typecons.d(3988): Error: cannot read uninitialized variable `payload` in CTFE            () @trusted { moveEmplace(value, _value.payload); }();

~~Also looks like the druntime implementation now lacks a bit of CTFE-ability that the Phobos one has.~~
I suppose that one’s on me, actually.

Copy link

Member

0xEAB commentedMar 14, 2025

I figure, emulating the previous behavior might be a hopeless endeavor.

Everything is built aroundmoveEmplaceImpl (the incompatible function in question), so in order to sneak in a wrapper around that, pretty much everything else this PR tries to remove has to stay in withmoveEmplaceImpl calls directed to the emulator.

AFAICT a more reasonable way forward would be to fixcore.lifetime by restoring the missing behavior ofemplaceMove() from Phobos. Though, I’m afraid that would be a good chunk of work.