From 72528ac83e0701deca313b3b7ccbc37d91e121e6 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 21 Oct 2020 19:12:53 +0200 Subject: [PATCH 1/4] core.lifetime: Fix copyEmplace() wrt. shared qualifier A tiny follow-up to #3239. Trying to use it in std.variant showed this issue. --- src/core/lifetime.d | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/core/lifetime.d b/src/core/lifetime.d index 99588e9527..f65a7d3ae3 100644 --- a/src/core/lifetime.d +++ b/src/core/lifetime.d @@ -1231,12 +1231,12 @@ void copyEmplace(S, T)(ref S source, ref T target) @system // this check seems to fail for nested aggregates /* && __traits(compiles, (ref S src) { T tgt = src; }) */) { - import core.internal.traits : hasElaborateCopyConstructor, Unqual; + import core.internal.traits : hasElaborateCopyConstructor, Unconst, Unqual; 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)) @@ -1252,7 +1252,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 } @@ -1275,7 +1275,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; } } @@ -1286,7 +1286,7 @@ void copyEmplace(S, T)(ref S source, ref T target) @system } else { - cast() target = source; + *cast(Unconst!(T)*) &target = source; } } @@ -1326,6 +1326,25 @@ 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 (CoreUnittest) { private void testCopyEmplace(S, T)(const scope T* expected = null) From e37a24a6ea3049f74faa72dc943c9b5b6a54d749 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 21 Oct 2020 21:26:52 +0200 Subject: [PATCH 2/4] core.internal.traits: Add BaseElemOf!T Corresponding to the front-end's Type.baseElemOf() method. Use it for some existing traits in that module, minimally fixing/ breaking semantics for `hasIndirections` for empty static arrays (now always false, independent from base element type). --- src/core/internal/traits.d | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/core/internal/traits.d b/src/core/internal/traits.d index 6f135a8967..9e69f805fc 100644 --- a/src/core/internal/traits.d +++ b/src/core/internal/traits.d @@ -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) { @@ -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); } else static if (is(S == struct)) { @@ -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)) { @@ -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)) { @@ -311,6 +328,7 @@ template hasElaborateCopyConstructor(S) } static assert(hasElaborateCopyConstructor!S); + static assert(!hasElaborateCopyConstructor!(S[0][1])); static struct S2 { @@ -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)) { @@ -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 From 499aa03cf7384f83d4e5e072f7ae14c80661b722 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Wed, 21 Oct 2020 21:38:48 +0200 Subject: [PATCH 3/4] core.lifetime: Fix copyEmplace() wrt. mutability qualifiers --- src/core/lifetime.d | 57 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/src/core/lifetime.d b/src/core/lifetime.d index f65a7d3ae3..e9e3db06a9 100644 --- a/src/core/lifetime.d +++ b/src/core/lifetime.d @@ -1227,11 +1227,17 @@ 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, Unconst, 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() { @@ -1286,7 +1292,7 @@ void copyEmplace(S, T)(ref S source, ref T target) @system } else { - *cast(Unconst!(T)*) &target = source; + *cast(Unconst!(T)*) &target = *cast(Unconst!(T)*) &source; } } @@ -1345,6 +1351,25 @@ void copyEmplace(S, T)(ref S source, ref T target) @system static assert(!__traits(compiles, copyEmplace(ss, t))); } +// 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); + 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) @@ -1431,11 +1456,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 From 66b033f42b3021452238513a60d3e337aa535019 Mon Sep 17 00:00:00 2001 From: Martin Kinkelin Date: Sat, 31 Oct 2020 17:09:49 +0100 Subject: [PATCH 4/4] core.lifetime: Work around apparent DMD codegen issues in unittests --- src/core/lifetime.d | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/lifetime.d b/src/core/lifetime.d index e9e3db06a9..e6230b48a2 100644 --- a/src/core/lifetime.d +++ b/src/core/lifetime.d @@ -1351,6 +1351,8 @@ void copyEmplace(S, T)(ref S source, ref T target) @system 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 { @@ -1364,7 +1366,8 @@ void copyEmplace(S, T)(ref S source, ref T target) @system assert(t is s); copyEmplace(si, ti); - assert(ti is si); + 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))); @@ -1496,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); } }