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 20869 - move is overly trusting of opPostMove #7502

Merged
merged 1 commit into from
May 31, 2020

Conversation

MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented May 28, 2020

Remove the manual check whether move is @safe and instead let the compiler do the attribute interference by adding appropriate @trusted blocks.

Note: I could've extend the current checks trustedMoveImpl but that could easily miss other corner cases.

TLDR: Only use @trusted when necessary

@dlang-bot
Copy link
Contributor

dlang-bot commented May 28, 2020

Thanks for your pull request and interest in making D better, @MoonlightSentinel! 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
20869 enhancement std.algorithm.mutation : move is overly trusting of opPostMove

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7502"

@thewilsonator
Copy link
Contributor

Please target stable.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Looks great!

@MoonlightSentinel
Copy link
Contributor Author

Please target stable.

Is stable broken?

std/math.d(4423): Error: `core.math.yl2x` called with argument types `(real, int)` matches both:
../druntime/import/core/math.d(167):     `core.math.yl2x(float x, float y)`
and:
../druntime/import/core/math.d(169):     `core.math.yl2x(real x, real y)`
posix.mak:326: recipe for target 'generated/linux/debug/64/libphobos2.a' failed

std/algorithm/mutation.d Outdated Show resolved Hide resolved
@MoonlightSentinel MoonlightSentinel changed the base branch from stable to master May 29, 2020 08:11
@MoonlightSentinel MoonlightSentinel force-pushed the move-safe branch 4 times, most recently from ec78dd6 to f00c231 Compare May 29, 2020 09:55
@MoonlightSentinel
Copy link
Contributor Author

Rebased to master because this might break code relying on the invalid @trusted (and stable seems to be broken ATM).

Remove the manual check whether move is `@safe` and instead let the
compiler do the attribute interference by adding appropriate @trusted
blocks.

Note:
I could've extend the current checks `trustedMoveImpl` but that could
easily miss other corner cases.
@n8sh
Copy link
Member

n8sh commented May 31, 2020

Is stable broken?

The broken compilation is because it's using stable phobos with master druntime instead of stable with stable.

@dlang-bot dlang-bot merged commit a7bd1a3 into dlang:master May 31, 2020
@MoonlightSentinel MoonlightSentinel deleted the move-safe branch June 10, 2020 13:47
MoonlightSentinel added a commit to MoonlightSentinel/druntime that referenced this pull request Jun 10, 2020
n8sh pushed a commit to n8sh/druntime that referenced this pull request Mar 3, 2021
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.

6 participants