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

Fix dup and idup property in the presence of copy constructors #3139

Closed
RazvanN7 wants to merge 1 commit into from
Closed

Fix dup and idup property in the presence of copy constructors #3139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 71 additions & 1 deletion src/object.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3105,11 +3105,20 @@ private U[] _dup(T, U)(T[] a) // pure nothrow depends on postblit
import core.stdc.string : memcpy;

void[] arr = _d_newarrayU(typeid(T[]), a.length);
memcpy(arr.ptr, cast(const(void)*)a.ptr, T.sizeof * a.length);
static if (__traits(hasCopyConstructor, T))
{
_doCopyCtor(a, *cast(U[]*)&arr);
}
else
{
memcpy(arr.ptr, cast(const(void)*)a.ptr, T.sizeof * a.length);
}

auto res = *cast(U[]*)&arr;

static if (__traits(hasPostblit, T))
_doPostblit(res);

return res;
}

Expand Down Expand Up @@ -3340,6 +3349,14 @@ private void _doPostblit(T)(T[] arr)
}
}

private void _doCopyCtor(T, U)(T[] src, U[] dst)
{
foreach (i, ref elem; src)
{
dst[i].__ctor(elem);
Copy link
Contributor

@radcapricorn radcapricorn Jun 17, 2020

Choose a reason for hiding this comment

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

What if it throws? Or rather, what if it throws, exception is caught, and later GC collects the array? Will it call any dtors? All of them? On a partially-initialized array?

Copy link
Member

@andralex andralex Jun 18, 2020

Choose a reason for hiding this comment

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

Yah, this needs care:

size_t i = 0;
scope (failure)
{
    for (size_t j = 0; j < i; ++j)
        dst[j].__dtor();
    delete dst;
}
for (; i < src.length; ++i)
    dst[i].__ctor(elem);

Copy link
Member

@andralex andralex Jun 18, 2020

Choose a reason for hiding this comment

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

Well not delete :)

Copy link
Contributor

@radcapricorn radcapricorn Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

__xdtor. If exists. And probably, before this call, not a _d_newarrayU, but a _d_newarrayiT. ;)
...or emplace remaining initializers in case of failure.

...on third thought, calling dtors here is probably unnecessary. Just need to make sure the remainder is in .init state. Although, what of the failed element itself? Are we allowed to overwrite it with .init? I mean, the GC better call those dtors if it collects. Or do defensively call dtors and put .init? Bah...

Copy link
Contributor

@radcapricorn radcapricorn Jun 18, 2020

Choose a reason for hiding this comment

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

Direct call to __ctor doesn't seem to write .init. Bad things may happen (c).

Copy link
Contributor

@edi33416 edi33416 Jul 18, 2020

Choose a reason for hiding this comment

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

This should probably call _d_arrayctor. Otherwise, we'll need to replicate the logic there as well. It seems that _d_arrayctor already implements the logic necessary to handle exceptions.

Also, _d_arrayctor uses postblitRecurse which knows about the new copyCtor.

There is, however, an issue with _d_arrayctor: it does a memcpy before calling postblitRecurse, which defies the purpose of the copyCtor, but this is an easy fix (I'll open a PR for it)

}
}

@safe unittest
{
static struct S1 { int* p; }
Expand Down Expand Up @@ -3421,6 +3438,59 @@ private void _doPostblit(T)(T[] arr)
static assert(!__traits(compiles, () @safe { [].idup!Sunsafe; }));
}

@safe unittest
{
static struct ArrElem
{
int a;
this(int a)
{
this.a = a;
}
this(ref const ArrElem)
{
a = 2;
}
this(ref ArrElem) immutable
{
a = 3;
}
}

auto arr = [ArrElem(1), ArrElem(1)];

ArrElem[] b = arr.dup;
assert(b[0].a == 2 && b[1].a == 2);

immutable ArrElem[] c = arr.idup;
assert(c[0].a == 3 && c[1].a == 3);
}

@system unittest
{
static struct Sunpure { this(ref const typeof(this)) @safe nothrow {} }
static struct Sthrow { this(ref const typeof(this)) @safe pure {} }
static struct Sunsafe { this(ref const typeof(this)) @system pure nothrow {} }
static assert( __traits(compiles, () { [].dup!Sunpure; }));
static assert(!__traits(compiles, () pure { [].dup!Sunpure; }));
static assert( __traits(compiles, () { [].dup!Sthrow; }));
static assert(!__traits(compiles, () nothrow { [].dup!Sthrow; }));
static assert( __traits(compiles, () { [].dup!Sunsafe; }));
static assert(!__traits(compiles, () @safe { [].dup!Sunsafe; }));

// for idup to work on structs that have copy constructors, it is necessary
// that the struct defines a copy constructor that creates immutable objects
static struct ISunpure { this(ref const typeof(this)) immutable @safe nothrow {} }
static struct ISthrow { this(ref const typeof(this)) immutable @safe pure {} }
static struct ISunsafe { this(ref const typeof(this)) immutable @system pure nothrow {} }
static assert( __traits(compiles, () { [].idup!ISunpure; }));
static assert(!__traits(compiles, () pure { [].idup!ISunpure; }));
static assert( __traits(compiles, () { [].idup!ISthrow; }));
static assert(!__traits(compiles, () nothrow { [].idup!ISthrow; }));
static assert( __traits(compiles, () { [].idup!ISunsafe; }));
static assert(!__traits(compiles, () @safe { [].idup!ISunsafe; }));
}
Copy link
Contributor

@wilzbach wilzbach Jun 17, 2020

Choose a reason for hiding this comment

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

Shouldn't the tests also test that the code actually works, i.e. the copy constructor of all array elements gets called?

Copy link
Contributor Author

@RazvanN7 RazvanN7 Jun 17, 2020

Choose a reason for hiding this comment

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

I've added one test

Copy link
Contributor

@radcapricorn radcapricorn Jun 18, 2020

Choose a reason for hiding this comment

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

Also missing implementation, and a test for a struct that contains pointers and has a this(ref const typeof(this)) copy ctor. Current dup/idup won't compile with those (they should). __traits(hasCopyConstructor, T) is really not sufficient.


@safe unittest
{
static int*[] pureFoo() pure { return null; }
Expand Down