Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Conversation

@lesderid
Copy link
Contributor

@lesderid lesderid commented Jun 10, 2019

This PR implements the druntime side of DIP1014.

The accepted DIP is a little ambiguous about whether nothrow should be required on opPostMove (i.e. applied to __move_post_blt). Quoting:

opPostMove, if implemented, MUST be nothrow.

[...]
This proposal does require that nothrow be defined on opPostMove, because throwing would mean a change in the program flow from a place that appears to execute no code. There is a danger that this will be too confusing for the programmer to properly take into account.

[...]

The DIP author suggested the requirement could be eased to "a very hearty recommendation" because D moves "everywhere" and it is "impossible" to prevent it from doing so.

cc @Shachar

@Shachar
Copy link

Shachar commented Jun 11, 2019

I don't think I'm authorize to make a definite decision on this point, but here's my hearty recommendation. I'll give my rational.

Why it should require nothrow:

  • The rational given in the DIP still stands, and it is only through people actually using the functionality that we will find out how restrictive it is.
  • Changing behavior in the language is easier from code disallowing something to code allowing something, as that direction creates no backwards compatibility problems.

Why it should not require nothrow:

  • I fear that the implementation might become sacred, with the above rational lost to memory. This is especially since I am personally no longer involved in D.

So my suggestion is to require nothrow, but to document that this is an interim decision in both commit message and in the code itself, with reference to this discussion.

@lesderid lesderid marked this pull request as ready for review June 11, 2019 12:53
@12345swordy
Copy link
Contributor

cc @thewilsonator @TurkeyMan

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

Needs a rebase though.

@TurkeyMan
Copy link
Contributor

Yeah it'd be really good if this got merged!

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @lesderid! 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#2638"

@lesderid
Copy link
Contributor Author

@thewilsonator Rebased it.

What about the nothrow though?

@lesderid
Copy link
Contributor Author

What about the nothrow though?

Actually it's probably fine like this for now.

@JinShil
Copy link
Contributor

JinShil commented Jun 27, 2019

What about the nothrow though?

I suggest making choosing the most restrictive option for now; it can always be relaxed later after there's more experience with it.

@codecov
Copy link

codecov bot commented Jun 29, 2019

Codecov Report

Merging #2638 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2638      +/-   ##
==========================================
+ Coverage   74.27%   74.29%   +0.02%     
==========================================
  Files         149      149              
  Lines       16808    16819      +11     
==========================================
+ Hits        12484    12496      +12     
+ Misses       4324     4323       -1
Impacted Files Coverage Δ
src/core/internal/traits.d 60.86% <ø> (ø) ⬆️
src/object.d 67.44% <100%> (+0.3%) ⬆️
src/rt/aaA.d 88.01% <0%> (-0.28%) ⬇️
src/rt/util/container/treap.d 80.59% <0%> (ø) ⬆️
src/core/thread.d 82.97% <0%> (+0.1%) ⬆️
src/core/internal/spinlock.d 77.77% <0%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b592940...963b2ff. Read the comment docs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants