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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wilzbach
Copy link
Member

In dlang/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 move doesPointTo to druntime as proposed, s.t. we don't need to maintain two implementations?

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

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

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
Member Author

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.

@LightBender LightBender added the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
@LightBender
Copy link
Contributor

@thewilsonator this one is for you.

@0xEAB
Copy link
Member

0xEAB commented Mar 14, 2025

This one is painful because core.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 the if (false) { nonTrustedImpl(); } trustedImpl(); @safety hacks used in the code that are inaccessibly from the outside (private “impl” functions).

@0xEAB
Copy link
Member

0xEAB commented Mar 14, 2025

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

@0xEAB
Copy link
Member

0xEAB commented Mar 14, 2025

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.

@0xEAB
Copy link
Member

0xEAB commented Mar 14, 2025

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

Everything is built around moveEmplaceImpl (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 with moveEmplaceImpl calls directed to the emulator.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:Needs Rebase Merge:stalled Review:Salvage This PR needs work, but we want to keep it and it can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants