From 36fc09a576174f5c73d01343d53c61ba00281bc5 Mon Sep 17 00:00:00 2001 From: Denis Shelomovskij Date: Sat, 3 Nov 2012 21:19:15 +0400 Subject: [PATCH 1/4] Move `move` precondition to `in` block * also improve precondition skipping comment --- std/algorithm.d | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index e8c5b350631..604b8c7ee10 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1454,9 +1454,10 @@ Preconditions: $(D &source == &target || !pointsTo(source, source)) */ void move(T)(ref T source, ref T target) +in { assert(&source == &target || !pointsTo(source, source)); } +body { if (&source == &target) return; - assert(!pointsTo(source, source)); static if (is(T == struct)) { // Most complicated case. Destroy whatever target had in it @@ -1548,7 +1549,7 @@ unittest /// Ditto T move(T)(ref T source) { - // Can avoid to check aliasing. + // Can avoid to check aliasing here. T result = void; static if (is(T == struct)) From 67c1118e072e373b7470eee1ca6c869ada57af0c Mon Sep 17 00:00:00 2001 From: Denis Shelomovskij Date: Sat, 3 Nov 2012 21:42:41 +0400 Subject: [PATCH 2/4] Simplify and fix complicated and error prone `move` Issues fixed: * `move` is over-complicated * `move` doesn't work with qualified structs * `move` doesn't work with const/immutable non-struct types * for a nested struct, `move` tries to call destructor on possibly invalid struct filled by `init` except with a context pointer of `target` (such mix is unexpected and invalid e.g. if default constructor is disabled) * `move` fails do the correct mix of `init` and context pointer of `target` in previous point in some cases * lots of other issues I'm to tired to search in list here --- std/algorithm.d | 82 +++++++++++++++---------------------------------- 1 file changed, 24 insertions(+), 58 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index 604b8c7ee10..70fde0ae4c4 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1458,42 +1458,17 @@ in { assert(&source == &target || !pointsTo(source, source)); } body { if (&source == &target) return; - 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); + static if (hasElaborateDestructor!T) + destruct(target, false); - // 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); - } - } - } + static if (hasElaborateAssign!T || !isAssignable!T) + memcpy(cast(void*) &target, cast(const void*) &source, T.sizeof); 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; - // } - } + + static if (hasElaborateCopyConstructor!T || hasElaborateDestructor!T) + setToInitialState(source); } unittest @@ -1512,6 +1487,12 @@ unittest move(s11, s12); assert(s11.a == 10 && s11.b == 11 && s12.a == 10 && s12.b == 11); + shared S1 sharedS11, sharedS12; + move(sharedS11, sharedS12); + + const int constI11, constI12; + void constTest(in int ci1) { const ci2 = move(ci1); } + static struct S2 { int a = 1; int * b; } S2 s21 = { 10, null }; s21.b = new int; @@ -1551,37 +1532,17 @@ T move(T)(ref T source) { // Can avoid to check aliasing here. - T result = void; - static if (is(T == struct)) + static if (hasElaborateCopyConstructor!T || hasElaborateDestructor!T) { - // 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); - } - } + T result = void; + memcpy(cast(void*) &result, cast(const void*) &source, T.sizeof); + setToInitialState(source); + return result; } else { - // Primitive data (including pointers and arrays) or class - - // assignment works great - result = source; + return source; } - return result; } unittest @@ -1598,6 +1559,9 @@ unittest S1 s12 = move(s11); assert(s11.a == 10 && s11.b == 11 && s12.a == 10 && s12.b == 11); + shared S1 sharedS11, sharedS12 = move(sharedS11); + void constTest(in int ci1, in int ci2) { move(ci1, ci2); } + static struct S2 { int a = 1; int * b; } S2 s21 = { 10, null }; s21.b = new int; @@ -1665,6 +1629,8 @@ unittest// Issue 8057 int x; ~this() { + // Struct always can equal to its `init` + if(this == S.init) return; // Access to enclosing scope assert(n == 10); } From 6b7141345f15c4e2de3ffdbe07350009a11aba90 Mon Sep 17 00:00:00 2001 From: Denis Shelomovskij Date: Sat, 3 Nov 2012 23:14:16 +0400 Subject: [PATCH 3/4] Fix and improve `move` documentation --- std/algorithm.d | 59 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/std/algorithm.d b/std/algorithm.d index 70fde0ae4c4..9d5beaabedf 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1443,12 +1443,59 @@ unittest // move /** -Moves $(D source) into $(D target) via a destructive -copy. Specifically: $(UL $(LI If $(D hasAliasing!T) is true (see -$(XREF traits, hasAliasing)), then the representation of $(D source) -is bitwise copied into $(D target) and then $(D source = T.init) is -evaluated.) $(LI Otherwise, $(D target = source) is evaluated.)) See -also $(XREF exception, pointsTo). +Moves $(D source) into $(D target). + +Specifically: +$(UL + $(LI Does nothing if $(D &source is &target) (for the first overload only). + ) + $(LI Destroys $(D target) if needed (for the first overload only, see + $(XREF traits, hasElaborateDestructor)) + ) + $(LI Bitwise copies $(D source) into $(D target). + ) + $(LI If $(D hasElaborateCopyConstructor!T || hasElaborateDestructor!T) + is $(D true) (see $(XREF traits, hasElaborateCopyConstructor)), + then sets $(D source) to $(D T.init). + ) +) + +Note: + +move is $(RED dangerous) as it works for $(B every) type $(D T) whatever +qualifiers or $(D const)/$(D immutable) members it has. +So it's the caller responsibility to use move properly. + +Example: +--- +// Consider three logically equal functions except `f1` and `f2` require +// type `S` to not disable default construction. +void f1(S s) +{ + { S tmp; } // <- Destructor (if any) call for `S.init` here + g(s); +} + +void f2(S s) +{ + S moved; + move(s, moved); + g(moved); + // <- Same destructor call here +} + +void f3(S s) +{ + S moved = move(s); // <- Same destructor call here + // Also a temporary for `move` return value may be created, resulting + // in a postblit and, then, a destructor call for a bitwise copy of `s`, + // but it likely will be removed by optimizer (e.i. no temporary created). + // This optimization is called NRVO (Named Return Value Optimization). + g(moved); +} +--- + +See also $(XREF exception, pointsTo), $(GLOSSARY nrvo). Preconditions: $(D &source == &target || !pointsTo(source, source)) From e6a79dece7a787b6ba0eabb7c0b7d0b8800b27f4 Mon Sep 17 00:00:00 2001 From: Denis Shelomovskij Date: Sun, 4 Nov 2012 01:24:05 +0400 Subject: [PATCH 4/4] Incorporate performance optimization suggested by @blackwhale --- std/algorithm.d | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/std/algorithm.d b/std/algorithm.d index 9d5beaabedf..b2e45412aef 100644 --- a/std/algorithm.d +++ b/std/algorithm.d @@ -1504,7 +1504,13 @@ void move(T)(ref T source, ref T target) in { assert(&source == &target || !pointsTo(source, source)); } body { - if (&source == &target) return; + // Performance optimization: + // Do not compare addresses if we don't have side effects, + // will not need addresses in `memcpy`, and T fits in register. + static if (hasElaborateCopyConstructor!T || hasElaborateAssign!T || + hasElaborateDestructor!T || !isAssignable!T || + T.sizeof > size_t.sizeof) + if (&source == &target) return; static if (hasElaborateDestructor!T) destruct(target, false);