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

core.lifetime: Fix copyEmplace() wrt. shared/mutability qualifiers #3246

Merged
dlang-bot merged 4 commits into from
Nov 3, 2020
Merged

core.lifetime: Fix copyEmplace() wrt. shared/mutability qualifiers #3246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
72528ac
core.lifetime: Fix copyEmplace() wrt. shared qualifier
kinke Oct 21, 2020
e37a24a
core.internal.traits: Add BaseElemOf!T
kinke Oct 21, 2020
499aa03
core.lifetime: Fix copyEmplace() wrt. mutability qualifiers
kinke Oct 21, 2020
66b033f
core.lifetime: Work around apparent DMD codegen issues in unittests
kinke Oct 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions src/core/internal/traits.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,23 @@ template Unqual(T)
}
}

template BaseElemOf(T)
{
static if (is(T == E[N], E, size_t N))
alias BaseElemOf = BaseElemOf!E;
else
alias BaseElemOf = T;
}

unittest
{
static assert(is(BaseElemOf!(int) == int));
static assert(is(BaseElemOf!(int[1]) == int));
static assert(is(BaseElemOf!(int[1][2]) == int));
static assert(is(BaseElemOf!(int[1][]) == int[1][]));
static assert(is(BaseElemOf!(int[][1]) == int[]));
}

// [For internal use]
template ModifyTypePreservingTQ(alias Modifier, T)
{
Expand Down Expand Up @@ -250,9 +267,9 @@ if (is(T == class))
/// See $(REF hasElaborateMove, std,traits)
template hasElaborateMove(S)
{
static if (__traits(isStaticArray, S) && S.length)
static if (__traits(isStaticArray, S))
{
enum bool hasElaborateMove = hasElaborateMove!(typeof(S.init[0]));
enum bool hasElaborateMove = S.sizeof && hasElaborateMove!(BaseElemOf!S);
Copy link
Contributor

@RazvanN7 RazvanN7 Nov 1, 2020

Choose a reason for hiding this comment

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

Any reason why length was changed to sizeof?

Copy link
Contributor

@RazvanN7 RazvanN7 Nov 1, 2020

Choose a reason for hiding this comment

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

I don't understand how BaseElemOf!S is any different from typeof(S.init[0])

Copy link
Contributor Author

@kinke kinke Nov 1, 2020

Choose a reason for hiding this comment

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

The difference shows up for multi-dimensional static arrays - BaseElemOf!(float[10][20][30]) == float. - Extracting it as little helper IMO helps to emphasize the intent and may reduce recursive instantiations of these hasElaborate{Move,Destructor,CopyConstructor,Assign} traits; higher dimensions don't matter here except for one of them being 0 and thus making the whole size 0; that's why I've had to change from recursively checking the length of each dimension to checking S.sizeof when passing a static array type and then instantiating the same template once again with the base element type.

Copy link
Contributor

@RazvanN7 RazvanN7 Nov 1, 2020

Choose a reason for hiding this comment

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

Makes total sense. Maybe we should have a test for this so that it does not get modified in the future.

Copy link
Contributor Author

@kinke kinke Nov 1, 2020

Choose a reason for hiding this comment

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

Yes, some tests added.

}
else static if (is(S == struct))
{
Expand All @@ -269,9 +286,9 @@ template hasElaborateMove(S)
// std.traits.hasElaborateDestructor
template hasElaborateDestructor(S)
{
static if (__traits(isStaticArray, S) && S.length)
static if (__traits(isStaticArray, S))
{
enum bool hasElaborateDestructor = hasElaborateDestructor!(typeof(S.init[0]));
enum bool hasElaborateDestructor = S.sizeof && hasElaborateDestructor!(BaseElemOf!S);
}
else static if (is(S == struct))
{
Expand All @@ -287,9 +304,9 @@ template hasElaborateDestructor(S)
// std.traits.hasElaborateCopyDestructor
template hasElaborateCopyConstructor(S)
{
static if (__traits(isStaticArray, S) && S.length)
static if (__traits(isStaticArray, S))
{
enum bool hasElaborateCopyConstructor = hasElaborateCopyConstructor!(typeof(S.init[0]));
enum bool hasElaborateCopyConstructor = S.sizeof && hasElaborateCopyConstructor!(BaseElemOf!S);
}
else static if (is(S == struct))
{
Expand All @@ -311,6 +328,7 @@ template hasElaborateCopyConstructor(S)
}

static assert(hasElaborateCopyConstructor!S);
static assert(!hasElaborateCopyConstructor!(S[0][1]));

static struct S2
{
Expand All @@ -332,9 +350,9 @@ template hasElaborateCopyConstructor(S)

template hasElaborateAssign(S)
{
static if (__traits(isStaticArray, S) && S.length)
static if (__traits(isStaticArray, S))
{
enum bool hasElaborateAssign = hasElaborateAssign!(typeof(S.init[0]));
enum bool hasElaborateAssign = S.sizeof && hasElaborateAssign!(BaseElemOf!S);
}
else static if (is(S == struct))
{
Expand All @@ -352,8 +370,8 @@ template hasIndirections(T)
{
static if (is(T == struct) || is(T == union))
enum hasIndirections = anySatisfy!(.hasIndirections, Fields!T);
else static if (__traits(isStaticArray, T) && is(T : E[N], E, size_t N))
enum hasIndirections = is(E == void) ? true : hasIndirections!E;
else static if (is(T == E[N], E, size_t N))
enum hasIndirections = T.sizeof && is(E == void) ? true : hasIndirections!(BaseElemOf!E);
else static if (isFunctionPointer!T)
enum hasIndirections = false;
else
Expand Down
95 changes: 80 additions & 15 deletions src/core/lifetime.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1227,16 +1227,22 @@ pure nothrow @safe /* @nogc */ unittest
* target = uninitialized value to be initialized with a copy of source
*/
void copyEmplace(S, T)(ref S source, ref T target) @system
if (is(immutable S == immutable T)
// this check seems to fail for nested aggregates
/* && __traits(compiles, (ref S src) { T tgt = src; }) */)
if (is(immutable S == immutable T))
{
import core.internal.traits : hasElaborateCopyConstructor, Unqual;
import core.internal.traits : BaseElemOf, hasElaborateCopyConstructor, Unconst, Unqual;

// cannot have the following as simple template constraint due to nested-struct special case...
static if (!__traits(compiles, (ref S src) { T tgt = src; }))
{
alias B = BaseElemOf!T;
enum isNestedStruct = is(B == struct) && __traits(isNested, B);
static assert(isNestedStruct, "cannot copy-construct " ~ T.stringof ~ " from " ~ S.stringof);
}

void blit()
{
import core.stdc.string : memcpy;
memcpy(cast(void*) &target, &source, T.sizeof);
memcpy(cast(Unqual!(T)*) &target, cast(Unqual!(T)*) &source, T.sizeof);
}

static if (is(T == struct))
Expand All @@ -1252,7 +1258,7 @@ void copyEmplace(S, T)(ref S source, ref T target) @system
static if (__traits(isNested, T))
{
// copy context pointer
cast() target.tupleof[$-1] = cast(typeof(target.tupleof[$-1])) source.tupleof[$-1];
*(cast(void**) &target.tupleof[$-1]) = cast(void*) source.tupleof[$-1];
}
target.__ctor(source); // invoke copy ctor
}
Expand All @@ -1275,7 +1281,7 @@ void copyEmplace(S, T)(ref S source, ref T target) @system
{
// destroy, in reverse order, what we've constructed so far
while (i--)
destroy(*cast(Unqual!(E)*) &target[i]);
destroy(*cast(Unconst!(E)*) &target[i]);
throw e;
}
}
Expand All @@ -1286,7 +1292,7 @@ void copyEmplace(S, T)(ref S source, ref T target) @system
}
else
{
cast() target = source;
*cast(Unconst!(T)*) &target = *cast(Unconst!(T)*) &source;
}
}

Expand Down Expand Up @@ -1326,6 +1332,47 @@ void copyEmplace(S, T)(ref S source, ref T target) @system
assert(target.x == 42);
}

// preserve shared-ness
@system pure nothrow unittest
{
auto s = new Object();
auto ss = new shared Object();

Object t;
shared Object st;

copyEmplace(s, t);
assert(t is s);

copyEmplace(ss, st);
assert(st is ss);

static assert(!__traits(compiles, copyEmplace(s, st)));
static assert(!__traits(compiles, copyEmplace(ss, t)));
}

version (DigitalMars) version (X86) version (Posix) version = DMD_X86_Posix;

// don't violate immutability for reference types
@system pure nothrow unittest
{
auto s = new Object();
auto si = new immutable Object();

Object t;
immutable Object ti;

copyEmplace(s, t);
assert(t is s);

copyEmplace(si, ti);
version (DMD_X86_Posix) { /* wrongly fails without -O */ } else
assert(ti is si);

static assert(!__traits(compiles, copyEmplace(s, ti)));
static assert(!__traits(compiles, copyEmplace(si, t)));
}

version (CoreUnittest)
{
private void testCopyEmplace(S, T)(const scope T* expected = null)
Expand Down Expand Up @@ -1412,11 +1459,23 @@ version (CoreUnittest)
}
}

S source = S(666);
immutable S target = void;
copyEmplace(source, target);
assert(target is source);
assert(copies == 1);
{
copies = 0;
S source = S(123);
immutable S target = void;
copyEmplace(source, target);
assert(target is source);
assert(copies == 1);
}

{
copies = 0;
immutable S[1] source = [immutable S(456)];
S[1] target = void;
copyEmplace(source, target);
assert(target[0] is source[0]);
assert(copies == 1);
}
}

// destruction of partially copied static array
Expand All @@ -1440,8 +1499,14 @@ version (CoreUnittest)
}
catch (Exception)
{
assert(S.deletions == [ 4, 3, 2, 1 ] ||
S.deletions == [ 4 ]); // FIXME: happens with -O
static immutable expectedDeletions = [ 4, 3, 2, 1 ];
version (DigitalMars)
{
assert(S.deletions == expectedDeletions ||
S.deletions == [ 4 ]); // FIXME: happens with -O
}
else
assert(S.deletions == expectedDeletions);
}
}

Expand Down