core.lifetime: Add copyEmplace() to simulate copy-construction#3239
core.lifetime: Add copyEmplace() to simulate copy-construction#3239dlang-bot merged 4 commits intodlang:masterfrom
Conversation
|
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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 referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + druntime#3239" |
cced01b to
0ed01fd
Compare
With a function signature analogous to moveEmplace() - source ref first, then target ref. Allowing non-mutable target types T is required to invoke the correct copy constructor, see the tests. std.variant has a similar implementation (but insufficient wrt. copy constructors), and there might be more potential usage sites in druntime/Phobos/user code. Destructing a partially copied static array has been adopted from core.internal.array.construction._d_arrayctor().
For copy-constructing a new dynamic array, either from a slice or a single element. This makes the implementation use copy constructors now.
For copy-constructing the appended element(s). This makes the implementation use copy constructors now.
|
Looks fine to me. cc @RazvanN7 |
A tiny follow-up to dlang#3239. Trying to use it in std.variant showed this issue.
A tiny follow-up to dlang#3239. Trying to use it in std.variant showed this issue.
A tiny follow-up to dlang#3239. Trying to use it in std.variant showed this issue.
| * source = value to be copied into target | ||
| * target = uninitialized value to be initialized with a copy of source | ||
| */ | ||
| void copyEmplace(S, T)(ref S source, ref T target) @system |
There was a problem hiding this comment.
Why is this marked as @system?
There was a problem hiding this comment.
I guess it was because of overwriting target, without destructing it first, so it's dangerous.
There was a problem hiding this comment.
Oh and because it accepts non-mutable target types T too.
With a function signature analogous to
moveEmplace()- source ref first, then target ref. Allowing non-mutable target types T is required to invoke the correct copy constructor, see the tests.std.varianthas a similar implementation (but insufficient wrt. copy constructors), and there might be more potential usage sites indruntime/Phobos/user code.
Destructing a partially copied static array has been adopted from copy-constructing a new dynamic array:
druntime/src/core/internal/array/construction.d
Lines 49 to 71 in b7391cd