This is the feedback thread for the first round of Community Review for DIP 1018, "The Copy Constructor":

https://github.com/dlang/DIPs/blob/07da1f2cabc8b1bc3ad66034598a133e5ad13356/DIPs/DIP1018.md

All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on January 4, or when I make a post declaring it complete. (This time I'm extending the review period by a few days because of the holidays.)

At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round of community review. Otherwise, it will be queued for the Final Review and Formal Assessment by the language maintainers.

Please familiarize yourself with the documentation for the Community Review before participating.

https://github.com/dlang/DIPs/blob/master/PROCEDURE.md#community-review

Thanks in advance to all who participate.

I think const postblit can be done by creating shadow copy of instance fields (but still physically located in the field).

struct A
{
    int b;
    this(this) immutable
    {
        immutable int shadow_b; //reads go here
        //b++
        immutable int t=shadow_b;
        shadow_b.__dtor();
        b.__ctor(t+1);
    }
}

Type system would treat them as distinct instances, but would destroy shadow copy right before assignment, after which shadow copy's lifetime ends and it goes out of scope completely, which will also invalidate any retained pointers if any.

On Wednesday, 19 December 2018 at 11:53:12 UTC, Kagamin wrote:
> I think const postblit can be done by creating shadow copy of instance fields (but still physically located in the field).
>
> struct A
> {
>     int b;
>     this(this) immutable
>     {
>         immutable int shadow_b; //reads go here
>         //b++
>         immutable int t=shadow_b;
>         shadow_b.__dtor();
>         b.__ctor(t+1);
>     }
> }
>
> Type system would treat them as distinct instances, but would destroy shadow copy right before assignment, after which shadow copy's lifetime ends and it goes out of scope completely, which will also invalidate any retained pointers if any.

This makes the blitting useless. Look at it this way: the compiler blitts the struct (in this case, `b`), then it makes another copy of it (`immutable int t = b) solely for the purpose of destroying the field for the former instance (!!!!!!). All this copies become useless when using the copy constructor.

Cheers,
RazvanN

On Wednesday, 19 December 2018 at 11:53:12 UTC, Kagamin wrote:
> I think const postblit can be done by creating shadow copy of instance fields (but still physically located in the field).
>
> struct A
> {
>     int b;
>     this(this) immutable
>     {
>         immutable int shadow_b; //reads go here
>         //b++
>         immutable int t=shadow_b;
>         shadow_b.__dtor();
>         b.__ctor(t+1);
>     }
> }
>
> Type system would treat them as distinct instances, but would destroy shadow copy right before assignment, after which shadow copy's lifetime ends and it goes out of scope completely, which will also invalidate any retained pointers if any.

You should not be resorting to calling the hidden symbol .__dtor, .__ctor in your production code as that is just asking for trouble. Which it is not guarantee to grant the behavior that you desire.

Alex

On Wednesday, 19 December 2018 at 14:34:37 UTC, 12345swordy wrote:
> On Wednesday, 19 December 2018 at 11:53:12 UTC, Kagamin wrote:
>> [...]
>
> You should not be resorting to calling the hidden symbol .__dtor, .__ctor in your production code as that is just asking for trouble. Which it is not guarantee to grant the behavior that you desire.
>
> Alex

I think that he was suggesting that that is compiler generated code.

On Wednesday, 19 December 2018 at 13:40:41 UTC, RazvanN wrote:
> All this copies become useless when using the copy constructor.

A copy constructor is very verbose though, imagine a copy constructor for a struct with 30 fields, where it copies 29 fields and sets the 30th to null. Postblit is observation of pattern that copy constructor usually changes only a small part of the struct. In this synthetic example copying wouldn't make much difference:
for this code:
---
struct A
{
    int b;
    void inc()
    {
        const int shadow_b=b;
        //b++
        const int t=shadow_b;
        b=t+1;
    }
}

A f(ref A a)
{
    A b=a;
    b.inc();
    return b;
}
---
`ldc -Os` generates this code for function f:
---
	movl	(%rdi), %eax
	addl	$1, %eax
	retq
---

On Thursday, 20 December 2018 at 14:41:49 UTC, Kagamin wrote:
> On Wednesday, 19 December 2018 at 13:40:41 UTC, RazvanN wrote:
>> [...]
>
> A copy constructor is very verbose though, imagine a copy constructor for a struct with 30 fields, where it copies 29 fields and sets the 30th to null.

For that you can use metaprogramming:

static foreach (i, ref field; src.tupleof)
    this.tupleof[i] = field;

this.field_of_choice = null;

>  Postblit is observation of pattern that copy constructor usually changes only a small part of the struct. In this synthetic example copying wouldn't make much difference:
> for this code:
> ---
> struct A
> {
>     int b;
>     void inc()
>     {
>         const int shadow_b=b;
>         //b++
>         const int t=shadow_b;
>         b=t+1;
>     }
> }
>
> A f(ref A a)
> {
>     A b=a;
>     b.inc();
>     return b;
> }
> ---
> `ldc -Os` generates this code for function f:
> ---
> 	movl	(%rdi), %eax
> 	addl	$1, %eax
> 	retq
> ---

On Thu, 20 Dec 2018 14:41:49 +0000, Kagamin wrote:
> On Wednesday, 19 December 2018 at 13:40:41 UTC, RazvanN wrote:
>> All this copies become useless when using the copy constructor.
> 
> A copy constructor is very verbose though, imagine a copy constructor for a struct with 30 fields, where it copies 29 fields and sets the 30th to null.

this.tupleof = other.tupleof;
this.field30 = null;

Pretty succinct.

On Thursday, 20 December 2018 at 16:53:29 UTC, Neia Neutuladh wrote:
> this.tupleof = other.tupleof;
> this.field30 = null;

This will initialize field30 twice, won't it?

On Tuesday, 18 December 2018 at 14:51:39 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1018, "The Copy Constructor"

1). I do not like the ability to specify a mutable copy source. Under no circumstance should the code like

A a;
A fun()
{
    return a;      // lowered to return tmp.copyCtor(a)
}

void main()
{
    A b = fun();    // the return value of fun() is moved to the location of b
}

be allowed to modify the value of a. This is an absolute pain to read\debug, and I would instead like to see a mandatory const\immutable on the source reference.
Copy operation should not modify the source, or it should not be called a copy. If we are talking about a typical smart pointer struct (Refcounted), copy still should not modify the source. It is D's transitive const\immutable that is a problem here and it must be explicitly casted away by the developer.

Only const also mitigates the combinatorial mess caused by 4 combinations of mutable\immutable copy constructors, since immutable is implicitly converted to const.


2). "A declaration is a copy constructor declaration if it is a constructor declaration that takes only one non-default parameter by reference that is of the same type as typeof(this), followed by any number of default parameters..."

If you need other parameters, you are not performing a copy. Copy constructor needs no additional parameters. If the semantics of your domain problem involve parametrized post-copy operations, the code should state that explicitly - by using specialized properly-named methods, that notify the reader about this particularity.


3). Section "Copy constructor usage", I don't understand the difference:

void main()
{
    A a;
    A b = a; // copy constructor gets called
    b = a;   // assignment, not initialization
}

and

void main()
{
    A a;
    a = fun();      // NRVO - no copy constructor call
    A b; // b is initialized after this semicolon.
    // why is this one not an assignment, but initialization?
    // do we have the difference formally and consistently defined for structs?
    b = gun();      // NRVO cannot be performed, copy constructor is called
}