Skip to content

Move improvements #922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
monarchdodra wants to merge 4 commits into from
Closed

Move improvements #922

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
a61dff0
Fix for move and attributes that end in "this"
monarchdodra Oct 31, 2012
98678a5
Consolidating move
monarchdodra Oct 31, 2012
aa9f3e4
hasContextPointer trait (internal)
monarchdodra Nov 3, 2012
5bdd22a
Move support for static arrays
monarchdodra Nov 3, 2012
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
188 changes: 123 additions & 65 deletions std/algorithm.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -1692,6 +1633,123 @@ 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);

//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);
}

/*
Like move, but assumes that source does not contain meaningful content.

Currently private
*/
private void uninitializedMove(T)(ref T source, ref T target)
{
//memcpy *always* works great!
memcpy(&target, &source, T.sizeof);
Copy link
Member

@DmitryOlshansky DmitryOlshansky Nov 4, 2012

Choose a reason for hiding this comment

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

What that's comment supposed to 'mean'?
//memcpy always works great!

It doesn't in CTFE and otherwise this feels like arbitrary noise.


// 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;
deinitialize(source, empty);
Copy link
Collaborator Author

@monarchdodra monarchdodra Nov 3, 2012

Choose a reason for hiding this comment

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

This will fail with @disable this. I'm thinking of replacing with this:

            if(auto p = typeid(T).init().ptr)
                deinitialize(source, *cast(T*)p);
            else
            {
                static if (hasContextPointer!T)
                {
                    static ubyte[T.sizeof] empty;
                    deinitialize(source, *cast(T*)empty.ptr);
                }
                else
                    memset(&source, 0, T.sizeof);
            }

But I'd rather do this in another pull, and concentrate on static arrays/structs deinitialization correctness for now.

Copy link
Member

@DmitryOlshansky DmitryOlshansky Nov 4, 2012

Choose a reason for hiding this comment

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

Blitting zeros with memset doesn't look right.
T.init is accessable with @disable this.

Copy link
Member

@DmitryOlshansky DmitryOlshansky Nov 4, 2012

Choose a reason for hiding this comment

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

NVM, I must be tired given that line above
auto p = typeid(T).init().ptr

}
}

/*
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 static if (is(T == struct) && hasContextPointer!T)
{
//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
/**
For each element $(D a) in $(D src) and each element $(D b) in $(D
Expand Down
57 changes: 57 additions & 0 deletions std/traits.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down