From a61dff010c0e88cf021a0070c965a46b75f99f55 Mon Sep 17 00:00:00 2001 From: monarch dodra Date: Wed, 31 Oct 2012 10:56:35 +0100 Subject: [PATCH 1/4] Fix for move and attributes that end in "this" As pointed out by @denis-sh --- std/algorithm.d | 50 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index e8c5b350631..62d43955380 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1471,7 +1471,7 @@ void move(T)(ref T source, ref T target) { static T empty; static if (T.tupleof.length > 0 && - T.tupleof[$-1].stringof.endsWith("this")) + T.tupleof[$-1].stringof.endsWith(".this")) { // If T is nested struct, keep original context pointer memcpy(&source, &empty, T.sizeof - (void*).sizeof); @@ -1563,7 +1563,7 @@ T move(T)(ref T source) { static T empty; static if (T.tupleof.length > 0 && - T.tupleof[$-1].stringof.endsWith("this")) + T.tupleof[$-1].stringof.endsWith(".this")) { // If T is nested struct, keep original context pointer memcpy(&source, &empty, T.sizeof - (void*).sizeof); @@ -1692,6 +1692,52 @@ unittest// Issue 8057 static assert(__traits(compiles, move(x, x) )); } +unittest +{ + //Make sure there is no funny business with empty structs + static struct S1 + {} + static struct S2 + { + ~this(){} + } + + S1 a1, b1; + S2 a2, b2; + a1.move(b1); + a2.move(b2); +} + +unittest +{ + //Test funny structure whose last attribute ends in "this"... + static struct Forest + { + int* lecythis; + this(this) + { + assert(0); + } + } + + int* p = [5].ptr; + Forest wa = Forest(p); + Forest wb; + wa.move(wb); + + //Make sure the lecythis was correctly moved + // (points the original pointer + assert(wb.lecythis == p); + + //Make sure the original Forest does not have a lecythis anymore + assert(wa.lecythis is null); + + Forest wc; + wc = move(wb); + assert(wc.lecythis == p); + assert(wb.lecythis is null); +} + // moveAll /** For each element $(D a) in $(D src) and each element $(D b) in $(D From 98678a5db5f673f49101fe380cdb01e12125aa89 Mon Sep 17 00:00:00 2001 From: monarch dodra Date: Wed, 31 Oct 2012 21:35:19 +0100 Subject: [PATCH 2/4] Consolidating move --- std/algorithm.d | 107 +++++++++++++++++++----------------------------- 1 file changed, 42 insertions(+), 65 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index 62d43955380..05ef63fd048 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1457,42 +1457,13 @@ void move(T)(ref T source, ref T target) { if (&source == &target) return; assert(!pointsTo(source, source)); - static if (is(T == struct)) - { - // Most complicated case. Destroy whatever target had in it - // and bitblast source over it - static if (hasElaborateDestructor!T) typeid(T).destroy(&target); - - memcpy(&target, &source, T.sizeof); - - // If the source defines a destructor or a postblit hook, we must obliterate the - // object in order to avoid double freeing and undue aliasing - static if (hasElaborateDestructor!T || hasElaborateCopyConstructor!T) + static if (is(T == struct) || isStaticArray!T) + static if (hasElaborateDestructor!T) { - static T empty; - static if (T.tupleof.length > 0 && - T.tupleof[$-1].stringof.endsWith(".this")) - { - // If T is nested struct, keep original context pointer - memcpy(&source, &empty, T.sizeof - (void*).sizeof); - } - else - { - memcpy(&source, &empty, T.sizeof); - } + // Most complicated case. Destroy whatever target had in it + typeid(T).destroy(&target); } - } - else - { - // Primitive data (including pointers and arrays) or class - - // assignment works great - target = source; - // static if (is(typeof(source = null))) - // { - // // Nullify the source to help the garbage collector - // source = null; - // } - } + uninitializedMove(source, target); } unittest @@ -1548,38 +1519,8 @@ unittest /// Ditto T move(T)(ref T source) { - // Can avoid to check aliasing. - T result = void; - static if (is(T == struct)) - { - // Can avoid destructing result. - - memcpy(&result, &source, T.sizeof); - - // If the source defines a destructor or a postblit hook, we must obliterate the - // object in order to avoid double freeing and undue aliasing - static if (hasElaborateDestructor!T || hasElaborateCopyConstructor!T) - { - static T empty; - static if (T.tupleof.length > 0 && - T.tupleof[$-1].stringof.endsWith(".this")) - { - // If T is nested struct, keep original context pointer - memcpy(&source, &empty, T.sizeof - (void*).sizeof); - } - else - { - memcpy(&source, &empty, T.sizeof); - } - } - } - else - { - // Primitive data (including pointers and arrays) or class - - // assignment works great - result = source; - } + uninitializedMove(source, result); return result; } @@ -1738,6 +1679,42 @@ unittest assert(wb.lecythis is null); } +/* +Like move, but assumes that source does not contain meaningful content. + +Currently private +*/ +private void uninitializedMove(T)(ref T source, ref T target) +{ + static if (is(T == struct)) + { + memcpy(&target, &source, T.sizeof); + + // If the source defines a destructor or a postblit hook, we must obliterate the + // object in order to avoid double freeing and undue aliasing + static if (hasElaborateDestructor!T || hasElaborateCopyConstructor!T) + { + static T empty; + static if (T.tupleof.length > 0 && + T.tupleof[$-1].stringof.endsWith(".this")) + { + // If T is nested struct, keep original context pointer + memcpy(&source, &empty, T.sizeof - (void*).sizeof); + } + else + { + memcpy(&source, &empty, T.sizeof); + } + } + } + else + { + // Primitive data (including pointers and arrays) or class - + // assignment works great + target = source; + } +} + // moveAll /** For each element $(D a) in $(D src) and each element $(D b) in $(D From aa9f3e427e1693a7bbef7828f45acc723daad84e Mon Sep 17 00:00:00 2001 From: monarch dodra Date: Sat, 3 Nov 2012 10:13:04 +0100 Subject: [PATCH 3/4] hasContextPointer trait (internal) --- std/traits.d | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/std/traits.d b/std/traits.d index 83b09bc3897..1392c5c2f1c 100644 --- a/std/traits.d +++ b/std/traits.d @@ -2568,6 +2568,63 @@ version(none) unittest static assert(hasMember!(R2!S, "T")); } +//For internal use +/* +Returns $(D true) if the type $(D T) contains an internal context pointer. +*/ +template hasContextPointer(S) +{ + static if (is(S == struct) || is(S == class)) + { + static if (S.tupleof.length > 0) + { + static if (S.tupleof[$-1].stringof.endsWith(".this")) + enum hasContextPointer = true; + else + enum hasContextPointer = anySatisfy!(.hasContextPointer, typeof(S.tupleof)); + } + else + enum hasContextPointer = false; + } + else static if(isStaticArray!S && S.length) + { + enum hasContextPointer = hasContextPointer!(typeof(S[0])); + } + else + { + enum hasContextPointer = false; + } +} + +unittest +{ + //basic types + static assert(!hasContextPointer!int); + static assert(!hasContextPointer!(void*)); + static assert(!hasContextPointer!string); + + //structs && (static)arrays + int a; + static struct S1{} + struct S2{this(int){++a;}} + + static assert(!hasContextPointer!S1); + static assert( hasContextPointer!S2); + static assert(!hasContextPointer!(S1[2])); + static assert( hasContextPointer!(S2[2])); + + static struct S3{S1 s1;} + static struct S4{S2 s2;} //Static struct aggregating a struct that has a context pointer (!) + static assert(!hasContextPointer!S3); + static assert( hasContextPointer!S4); + static assert(!hasContextPointer!(S3[2])); + static assert( hasContextPointer!(S4[2])); + + //classes: Bugged? + class C{this(){++a;}} + static assert(!hasContextPointer!C); +} + /** Retrieves the members of an enumerated type $(D enum E). From 5bdd22a211d1b233123a2320fc434932431fe8ca Mon Sep 17 00:00:00 2001 From: monarch dodra Date: Sat, 3 Nov 2012 10:25:07 +0100 Subject: [PATCH 4/4] Move support for static arrays Also adds support for nests context pointers --- std/algorithm.d | 75 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index 05ef63fd048..f187c0835be 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1672,11 +1672,25 @@ unittest //Make sure the original Forest does not have a lecythis anymore assert(wa.lecythis is null); - + Forest wc; wc = move(wb); assert(wc.lecythis == p); assert(wb.lecythis is null); + + //test static array move + int* p1 = [1].ptr; + int* p2 = [2].ptr; + Forest[2] trees; + trees[0].lecythis = p1; + trees[1].lecythis = p2; + + Forest[2] otherTrees; + move(trees, otherTrees); + + //verify that the pointers were actually moved (and not post-blitted) + assert(otherTrees[0].lecythis is p1); + assert(otherTrees[1].lecythis is p2); } /* @@ -1686,33 +1700,54 @@ Currently private */ private void uninitializedMove(T)(ref T source, ref T target) { - static if (is(T == struct)) - { - memcpy(&target, &source, T.sizeof); + //memcpy *always* works great! + memcpy(&target, &source, T.sizeof); - // If the source defines a destructor or a postblit hook, we must obliterate the - // object in order to avoid double freeing and undue aliasing + // If the source defines a destructor or a postblit hook, we must obliterate the + // object in order to avoid double freeing and undue aliasing + static if (is(T == struct) || isStaticArray!T) static if (hasElaborateDestructor!T || hasElaborateCopyConstructor!T) { static T empty; - static if (T.tupleof.length > 0 && - T.tupleof[$-1].stringof.endsWith(".this")) - { - // If T is nested struct, keep original context pointer - memcpy(&source, &empty, T.sizeof - (void*).sizeof); - } - else - { - memcpy(&source, &empty, T.sizeof); - } + deinitialize(source, empty); } +} + +/* +copies $(D source) over $(D target), while keeping any context pointers intact; + +Currently private +*/ +private void deinitialize(T)(ref T target, ref T source) +{ + static if (isStaticArray!T && hasContextPointer!T) + { + foreach(i; 0 .. T.length) + deinitialize(target[i], source[i]); } - else + else static if (is(T == struct) && hasContextPointer!T) { - // Primitive data (including pointers and arrays) or class - - // assignment works great - target = source; + //Our T has a context pointer. Is it here, or lower? + static if (T.tupleof[$-1].stringof.endsWith(".this")) + { + //It's here, but are there any *other*, lower? + static if (anySatisfy!(.hasContextPointer, typeof(T.tupleof[$-1]))) + //yes, deinitialize each inidividually + foreach(i; 0 .. T.tupleof.length - 1) + deinitialize(target.tupleof[i], source.tupleof[i]); + else + //nope, shortcut. + memcpy(&target, &source, T.sizeof - (void*).sizeof); + } + else + { + //It's not this, so it must be lower, deinitialize each inidividually + foreach(i; 0 .. T.tupleof.length) + deinitialize(target.tupleof[i], source.tupleof[i]); + } } + else + memcpy(&target, &source, T.sizeof); } // moveAll