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

Make _d_arrayctor and _d_arraysetctor aware of copy constructor #3177

Closed
edi33416 wants to merge 3 commits into from
Closed

Make _d_arrayctor and _d_arraysetctor aware of copy constructor #3177

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9111911
Make _d_arrayctor and _d_arraysetctor aware of copy constructor
edi33416 Aug 1, 2020
2145c0b
Copy ctor can be built from a differently qualified type
edi33416 Aug 3, 2020
e8e4be6
Add support for static array of structs
edi33416 Aug 20, 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
279 changes: 261 additions & 18 deletions src/core/internal/array/construction.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,35 @@
*/
module core.internal.array.construction;

package void cpCtorRecurse(S1, S2)(ref S1 to, ref S2 from)
if (is(S1 == struct) && is (immutable S1 == immutable S2))
{
static if (__traits(hasCopyConstructor, S1))
{
to.__ctor(from);
}
}

package void cpCtorRecurse(T, size_t n, U)(ref T[n] to, ref U[n] from)
{
import core.internal.destruction: destructRecurse;

static if (__traits(hasCopyConstructor, T))
{
size_t i;
scope(failure)
{
for (; i != 0; --i)
{
destructRecurse(to[i - 1]); // Don't care if it throws, as throwing in dtor can lead to UB
}
}

for (i = 0; i < to.length; ++i)
cpCtorRecurse(to[i], from[i]);
}
}

/**
* Does array initialization (not assignment) from another array of the same element type.
* Params:
Expand All @@ -21,7 +50,8 @@ module core.internal.array.construction;
* purity, and throwabilty checks. To prevent breaking existing code, this function template
* is temporarily declared `@trusted` until the implementation can be brought up to modern D expectations.
*/
Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted
Tarr _d_arrayctor(Tarr : T[], T, Uarr : U[], U)(return scope Tarr to, scope Uarr from) @trusted
if (is (immutable T == immutable U))
{
pragma(inline, false);
import core.internal.traits : hasElaborateCopyConstructor, Unqual;
Expand Down Expand Up @@ -51,8 +81,17 @@ Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted
size_t i;
try
{
for (i = 0; i < to.length; i++)
copyEmplace(from[i], to[i]);
static if (__traits(hasCopyConstructor, T))
{
cpCtorRecurse(to[i], from[i]);
}
else
{
auto elem = cast(Unqual!T*)&to[i];
// Copy construction is defined as bit copy followed by postblit.
memcpy(elem, &from[i], element_size);
Copy link
Contributor

@kinke kinke Aug 18, 2020

Choose a reason for hiding this comment

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

Not related to this PR, but memcpying each element separately is not optimal, and doesn't exploit the postblit advantage over copy ctors. The loop can additionally be completely elided if the type has no postblit.

Copy link
Contributor

@kinke kinke Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Btw, is this stuff actually used at all? The compiler still lowers to the non-templated extern(C) hook...

Edit: Seems totally unused in 2.093, incl. no usages in druntime/Phobos.

Copy link
Contributor Author

@edi33416 edi33416 Aug 18, 2020

Choose a reason for hiding this comment

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

You are right. This doesn't get lowered to by the compiler.
As you've pointed out, the compiler lowers to the array function from rt.arrayassign.

The course of action going forward is to hook the call to _d_arrayctor in dmd, as @RazvanN7 pointed out.

Before doing so, we'll have to convert _d_arrayassign, _d_arrayassign_l and _d_arrayassign_r to into templates and then change dmd to call the templated versions of _d_arrayassign and the current templated version of _d_arrayctor and their set cousins _d_arraysetctor and _d_arraysetassign

Copy link
Contributor Author

@edi33416 edi33416 Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not related to this PR, but memcpying each element separately is not optimal, and doesn't exploit the postblit advantage over copy ctors.

I believe the memcpying is done separately so it will be able to correctly destroy previously constructed elements in case a of a throw from the postblit of the current ith element

The loop can additionally be completely elided if the type has no postblit.

I think the memcopying should still be performed.

Copy link
Contributor

@kinke kinke Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

When performing a single memcpy, the 'unconstructed' elements >= i will have to be reset (edit: blitted) to T.init in the exception case, yes.

Copy link
Contributor Author

@edi33416 edi33416 Aug 18, 2020

Choose a reason for hiding this comment

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

So you're suggesting that the code should be changed to

try
    {
        static if (__traits(hasPostblit, T))
        {
            memcpy(to, from, element_size * to.length);
            for (i = 0; i < to.length; i++)
            {
                auto elem = cast(Unqual!T*)&to[i];
                postblitRecurse(*elem);
            }
        }
        else static if (__traits(hasCopyConstructor, T))
        {
            for (i = 0; i < to.length; i++)
            {
                to[i].__ctor(from[i]);
            }
        }
        else
        {
            memcpy(to, from, element_size * to.length);
        }
    catch (Exception o)
    {
        /* Destroy, in reverse order, what we've constructed so far
        */
        static if (__traits(hasPostblit, T) || !__traits(hasCopyConstructor, T))
        {
            for (int j = i; j < to.length; j++)
            {
                auto elem = cast(Unqual!T*)&to[i];
                memcpy(elem, &T.init, element_size);
            }
        }
        while (i--)
        {
            auto elem = cast(Unqual!T*)&to[i];
            destroy(*elem);
        }
        throw o;
    }

Copy link
Contributor

@kinke kinke Aug 18, 2020

Choose a reason for hiding this comment

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

Something similar to this, yes. Elements without postblit/cpctor don't need a try/catch, that's a plain memcpy. T.init is an rvalue and cannot be used for memcpy; there's some emplace version blitting the default initializer which can be used - in case the T.init reset is really necessary (the array wasn't successfully constructed, so is it going to be destructed at some point later? have the elements already been initialized with T.init in the first place?).

Copy link
Contributor Author

@edi33416 edi33416 Aug 20, 2020

Choose a reason for hiding this comment

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

I'd keep this as a separate PR in an attempt to keep the changes small

postblitRecurse(*elem);
}
}
catch (Exception o)
{
Expand Down Expand Up @@ -94,19 +133,26 @@ Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted
assert(arr1 == arr2);
}

// copy constructor
@safe unittest
{
int counter;
struct S
// Test that copy constructor works
static int counter;
static struct S
{
int val;
this(int val) { this.val = val; }
this(const scope ref S rhs)
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
}
this(ref typeof(this) rhs) immutable
{
val = rhs.val + 1; // just to check that we call the correct cpctor
}
}

S[4] arr1;
Expand All @@ -115,6 +161,22 @@ Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted

assert(counter == 4);
assert(arr1 == arr2);

immutable S[4] arr3;
_d_arrayctor(arr3[], arr2[]);

assert(arr3 == [S(1), S(2), S(3), S(4)]);

S[2][2] arr4;
S[2][2] arr5 = [[S(0), S(1)], [S(2), S(3)]];
_d_arrayctor(arr4[], arr5[]);
assert(counter == 8);
assert(arr4 == arr5);

immutable S[2][2] arr6;
_d_arrayctor(arr6[], arr5[]);

assert(arr6 == [[S(1), S(2)], [S(3), S(4)]]);
}

@safe nothrow unittest
Expand Down Expand Up @@ -172,6 +234,85 @@ Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted
assert(counter == 4);
}

@safe nothrow unittest
{
// Test that throwing copy constructor works
static int counter;
bool didThrow;

static struct Throw
{
int val;
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
if (counter == 2)
throw new Exception("");
}
}
try
{
Throw[4] a;
Throw[4] b = [Throw(1), Throw(2), Throw(3), Throw(4)];
_d_arrayctor(a[], b[]);
}
catch (Exception)
{
didThrow = true;
}
assert(didThrow);
assert(counter == 2);

didThrow = false;
counter = 0;
try
{
Throw[2][2] a;
Throw[2][2] b = [[Throw(1), Throw(2)], [Throw(3), Throw(4)]];
_d_arrayctor(a[], b[]);
}
catch (Exception)
{
didThrow = true;
}
assert(didThrow);
assert(counter == 2);

// Test that `nothrow` works
didThrow = false;
counter = 0;
static struct NoThrow
{
int val;
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
}
}
try
{
NoThrow[4] a;
NoThrow[4] b = [NoThrow(1), NoThrow(2), NoThrow(3), NoThrow(4)];
_d_arrayctor(a[], b[]);
}
catch (Exception)
{
didThrow = false;
}
assert(!didThrow);
assert(counter == 4);
}

/**
* Do construction of an array.
* ti[count] p = value;
Expand All @@ -183,7 +324,8 @@ Tarr _d_arrayctor(Tarr : T[], T)(return scope Tarr to, scope Tarr from) @trusted
* purity, and throwabilty checks. To prevent breaking existing code, this function template
* is temporarily declared `@trusted` until the implementation can be brought up to modern D expectations.
*/
void _d_arraysetctor(Tarr : T[], T)(scope Tarr p, scope ref T value) @trusted
void _d_arraysetctor(Tarr : T[], T, U)(scope Tarr p, scope ref U value) @trusted
if (is (immutable T == immutable U))
{
pragma(inline, false);
import core.internal.traits : Unqual;
Expand All @@ -192,8 +334,21 @@ void _d_arraysetctor(Tarr : T[], T)(scope Tarr p, scope ref T value) @trusted
size_t i;
try
{
for (i = 0; i < p.length; i++)
copyEmplace(value, p[i]);
foreach (i; 0 .. p.length)
{
static if (__traits(hasCopyConstructor, T))
{
cpCtorRecurse(p[walker], value);
}
else
{
auto elem = cast(Unqual!T*)&p[walker];
// Copy construction is defined as bit copy followed by postblit.
memcpy(elem, &value, element_size);
postblitRecurse(*elem);
}
walker++;
}
}
catch (Exception o)
{
Expand Down Expand Up @@ -228,26 +383,48 @@ void _d_arraysetctor(Tarr : T[], T)(scope Tarr p, scope ref T value) @trusted
assert(arr == [S(1234), S(1234), S(1234), S(1234)]);
}

// copy constructor
@safe unittest
{
int counter;
struct S
// Test that copy constructor works
static int counter;
static struct S
{
int val;
this(int val) { this.val = val; }
this(const scope ref S rhs)
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
}
this(ref typeof(this) rhs) immutable
{
val = rhs.val + 1; // just to check that we call the correct cpctor
}
}

S[4] arr;
S s = S(1234);
_d_arraysetctor(arr[], s);
assert(counter == arr.length);
assert(arr == [S(1234), S(1234), S(1234), S(1234)]);
immutable S[4] arr2;
_d_arraysetctor(arr2[], s);
assert(arr2 == [S(1235), S(1235), S(1235), S(1235)]);

counter = 0;
S[2] s2 = [S(1234), S(1234)];
S[2][2] arr3;

_d_arraysetctor(arr3[], s2);
assert(counter == arr.length);
assert(arr3 == [[S(1234), S(1234)], [S(1234), S(1234)]]);

immutable S[2][2] arr4;
_d_arraysetctor(arr4[], s2);
assert(arr4 == [[S(1235), S(1235)], [S(1235), S(1235)]]);
}

@safe nothrow unittest
Expand All @@ -268,8 +445,8 @@ void _d_arraysetctor(Tarr : T[], T)(scope Tarr p, scope ref T value) @trusted
try
{
Throw[4] a;
Throw[4] b = [Throw(1), Throw(2), Throw(3), Throw(4)];
_d_arrayctor(a[], b[]);
Throw b = Throw(1);
_d_arraysetctor(a[], b);
}
catch (Exception)
{
Expand Down Expand Up @@ -305,3 +482,69 @@ void _d_arraysetctor(Tarr : T[], T)(scope Tarr p, scope ref T value) @trusted
assert(!didThrow);
assert(counter == 4);
}

@safe nothrow unittest
{
// Test that throwing copy constructor works
static int counter;
bool didThrow;
static struct Throw
{
int val;
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
if (counter == 2)
throw new Exception("Oh no.");
}
}
try
{
Throw[4] a;
Throw b = Throw(1);
_d_arraysetctor(a[], b);
}
catch (Exception)
{
didThrow = true;
}
assert(didThrow);
assert(counter == 2);


// Test that `nothrow` works
didThrow = false;
counter = 0;
static struct NoThrow
{
int val;
this(int v)
{
val = v;
}
this(ref typeof(this) rhs)
{
val = rhs.val;
counter++;
}
}
try
{
NoThrow[4] a;
NoThrow b = NoThrow(1);
_d_arraysetctor(a[], b);
foreach (ref e; a)
assert(e == NoThrow(1));
}
catch (Exception)
{
didThrow = false;
}
assert(!didThrow);
assert(counter == 4);
}