http://d.puremagic.com/issues/show_bug.cgi?id=9732

           Summary: Do not call opAssign() for the first assignment to
                    member in the constructor
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Keywords: performance
          Severity: enhancement
          Priority: P3
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: acehreli@yahoo.com


--- Comment #0 from Ali Cehreli <acehreli@yahoo.com> 2013-03-15 17:16:46 PDT ---
This is about struct members of both structs and classes.

It is a common idiom in constructors to assign rvalues to members:

    this(int i)
    {
        this.inner = Inner(i);    // <-- assign from rvalue
    }

If 'Inner' above is a struct that has an rvalue opAssign() defined, currently dmd compiles a call to that opAssign(). However, it is safe to elide that call for the first such assignment in the constructor. Rather, the compiler can blit the rvalue on top of the .init state of the member.

The following program demonstrates the two calls to opAssign() that could be
elided:

import std.stdio;

struct Inner
{
    int i;

    void opAssign(Inner rhs)
    {
        writeln("rvalue opAssign called");
    }
}

struct OuterStruct
{
    Inner inner;

    this(int i)
    {
        writeln("Assigning to OuterStruct.inner");
        this.inner = Inner(i);
    }
}

class OuterClass
{
    Inner inner;

    this(int i)
    {
        writeln("Assigning to OuterClass.inner");
        this.inner = Inner(i);
    }
}

void main()
{
    writeln("\nConstructing main.inner");
    auto inner = Inner(42);

    writeln("\nConstructing main.outer_struct");
    auto outer_struct = OuterStruct(43);

    writeln("\nConstructing main.outer_class");
    auto outer_class = new OuterClass(44);
}

The output:

  Constructing main.inner

  Constructing main.outer_struct
  Assigning to OuterStruct.inner
  rvalue opAssign called           // <-- could be elided

  Constructing main.outer_class
  Assigning to OuterClass.inner
  rvalue opAssign called           // <-- could be elided

Note that if dmd merely blitted the rvalue, the two lines "rvalue opAssign called" would not be printed.

Thank you,
Ali

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732


Maxim Fomin <maxim@maxim-fomin.ru> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |maxim@maxim-fomin.ru


--- Comment #1 from Maxim Fomin <maxim@maxim-fomin.ru> 2013-03-15 22:36:57 PDT ---
The problem is that opAssign can perform deep copy operations for which blitting isn't enough.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #2 from Ali Cehreli <acehreli@yahoo.com> 2013-03-16 22:25:36 PDT ---
I think you are right because the .init state of a member may not be as simple as one thinks. Although OuterStruct.inner must be known at compile time, it may have a non-trivial destructor that needs to be called:

struct Inner
{
    ~this()
    {
        // ... not-trivial destructor ...
    }

    // ...
}

struct OuterStruct
{
    Inner inner = Inner(specialInitValue);    // <-- compile-time .init

    this(int i)
    {
        this.inner = Inner(i);    // <-- further assignment
    }
}

Blitting the rvalue on top of OuterStruct.inner would not be right in that case.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732


monarchdodra@gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |monarchdodra@gmail.com


--- Comment #3 from monarchdodra@gmail.com 2013-03-17 07:14:20 PDT ---
(In reply to comment #2)
> I think you are right because the .init state of a member may not be as simple as one thinks. Although OuterStruct.inner must be known at compile time, it may have a non-trivial destructor that needs to be called:
> 
> struct Inner
> {
>     ~this()
>     {
>         // ... not-trivial destructor ...
>     }
> 
>     // ...
> }
> 
> struct OuterStruct
> {
>     Inner inner = Inner(specialInitValue);    // <-- compile-time .init
> 
>     this(int i)
>     {
>         this.inner = Inner(i);    // <-- further assignment
>     }
> }
> 
> Blitting the rvalue on top of OuterStruct.inner would not be right in that case.

I really don't think that's a problem. If anything, the default value should NOT be destroyed in the constructor. After all, it hasn't really even been initialized yet.

If anything, that's exactly how "aggregate construction" works: postblit each value over the current fields, but DON'T destroy the fields:

//--------
import std.stdio;

struct Inner
{
    int i;
    this(this){} //Postblit implies destruction of this.
    ~this()
    {
        writeln(i);
    }
}

struct Outer
{
    Inner inner = Inner(1);    // <-- compile-time .init
}

void main()
{
    auto inner = Inner(2);
    {
        writeln("****");
        auto outer = Outer(inner); // Overwrite with postblit, but don't call
destructor.
        writeln("****");
    }
    {
        writeln("****");
        auto outer = Outer(inner); // Overwrite with postblit, but don't call
destructor.
        outer.inner = inner;       // Call postblit, destroying previous state.
        writeln("****");
    }
}
//--------
****
**** //Notice no destroyer called here
2    //This is outer's destroyer destroying its inner
****
2    //This is our second blit.
****
2    //This is out second outer destroyer's destroying its inner
2    //This is our inner's destroyer
//--------

So yeah, my conclusion is: destroyers are not a problem to this proposal.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #4 from monarchdodra@gmail.com 2013-03-17 07:21:15 PDT ---
(In reply to comment #1)
> The problem is that opAssign can perform deep copy operations for which blitting isn't enough.

That's why we have postblit. If anything, I'd expect postblit to be safer then opAssign: postblit has no pre-state, whereas opAssign may make assumptions about it's current (run-time initialized?) state.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #5 from monarchdodra@gmail.com 2013-03-17 07:49:57 PDT ---
I like this ER, because Contruction != Assignement. I see 0 reasons to call an opAssign in a constructor. It's just wrong. Being able to clearly make this distinction is very important (IMO). There are actually a few low level (but important) bugs in phobos, where an opAssign is called when a constructor should have been called instead.

On the other hand, I fear that by mixing both notions under the same syntax, that we'll introduce subtle but hard to find bugs, or confuse users with complex rules:  how do you document the "first assignment"? What if there is a run-time "if" in there? What if you make a read of the variable before the first assignment?

I'm all for such an enhancement (I think it's important), but I'm not sold on the way it is proposed here.

...

I think I just had a crazy idea: how about allowing a single re-declaration inside the constructor to explicitly mean constuction as opposed to destruction. If you do this, then you may not use said field prior to the re-declaration.

This should be 100% safe, since D bans shadowing.

Examples:

//----
struct S(T)
{
    T a;
    T b;
}

this(??)
{
    T a = ...; //Construc a from ...
    b = ...; //Assign to b from ... over b's initial value
}

this(???)
{
    a = ...; //Imagine initialization of a here, or just use of a here:
    T a = ...; //Error, a has already been used by here
}

this(???)
{
    T a = ...; //OK
    T a = ...; //Error, redeclaration}
}
//----

I think this is both simple, but powerful, and easy to grasp.

It has the added benefit that (unlike C++):
 - You can declare variables and run code before constructing any members.
 - You can defer field construction to any point in time you like.
 - You can construct fields in any arbitrary order.

Thoughts?

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #6 from Maxim Fomin <maxim@maxim-fomin.ru> 2013-03-17 08:56:49 PDT ---
(In reply to comment #4)
> (In reply to comment #1)
> > The problem is that opAssign can perform deep copy operations for which blitting isn't enough.
> 
> That's why we have postblit. If anything, I'd expect postblit to be safer then opAssign: postblit has no pre-state, whereas opAssign may make assumptions about it's current (run-time initialized?) state.

Postblit does not cover cases when object assigned to struct is of different type - this is main purpose of overloading opAssign in addition to defining postblit.

Making assumptions about state of object on entering postblit and opAssign is very risky, you can effectively receive garbage.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #7 from Maxim Fomin <maxim@maxim-fomin.ru> 2013-03-17 09:21:59 PDT ---
(In reply to comment #3)
> I really don't think that's a problem. If anything, the default value should NOT be destroyed in the constructor. After all, it hasn't really even been initialized yet.

There are several corner cases when dtor gets default value. By the way, how you can distinguish whether object was recently initialized to its state was reseted?

> If anything, that's exactly how "aggregate construction" works: postblit each value over the current fields, but DON'T destroy the fields:
> 
> //--------
> import std.stdio;
> 
> struct Inner
> {
>     int i;
>     this(this){} //Postblit implies destruction of this.
>     ~this()
>     {
>         writeln(i);
>     }
> }
> 
> struct Outer
> {
>     Inner inner = Inner(1);    // <-- compile-time .init
> }
> 
> void main()
> {
>     auto inner = Inner(2);
>     {
>         writeln("****");
>         auto outer = Outer(inner); // Overwrite with postblit, but don't call
> destructor.
>         writeln("****");
>     }
>     {
>         writeln("****");
>         auto outer = Outer(inner); // Overwrite with postblit, but don't call
> destructor.
>         outer.inner = inner;       // Call postblit, destroying previous state.
>         writeln("****");
>     }
> }
> //--------
> ****
> **** //Notice no destroyer called here
> 2    //This is outer's destroyer destroying its inner
> ****
> 2    //This is our second blit.
> ****
> 2    //This is out second outer destroyer's destroying its inner
> 2    //This is our inner's destroyer
> //--------
> 
> So yeah, my conclusion is: destroyers are not a problem to this proposal.

I don't how this is related to the issue.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #8 from monarchdodra@gmail.com 2013-03-17 09:48:36 PDT ---
(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #1)
> > > The problem is that opAssign can perform deep copy operations for which blitting isn't enough.
> > 
> > That's why we have postblit. If anything, I'd expect postblit to be safer then opAssign: postblit has no pre-state, whereas opAssign may make assumptions about it's current (run-time initialized?) state.
> 
> Postblit does not cover cases when object assigned to struct is of different type - this is main purpose of overloading opAssign in addition to defining postblit.

Yes, but if the types don't match, then the discussion we're having here is irrelevant: The only way to avoid a call to opAssign is if the types match, in which case you can postblit.

> Making assumptions about state of object on entering postblit and opAssign is very risky, you can effectively receive garbage.

Yes, but when you postblit, you only have to worry about the state of the "rhs" object, since the "lhs" object has no prior state. With opAssign, you may have to worry about the state of the currently being assigned to object.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=9732



--- Comment #9 from monarchdodra@gmail.com 2013-03-17 09:53:04 PDT ---
(In reply to comment #7)
> (In reply to comment #3)
> > I really don't think that's a problem. If anything, the default value should NOT be destroyed in the constructor. After all, it hasn't really even been initialized yet.
> 
> There are several corner cases when dtor gets default value. By the way, how you can distinguish whether object was recently initialized to its state was reseted?
> 
> > If anything, that's exactly how "aggregate construction" works: postblit each value over the current fields, but DON'T destroy the fields:
> > 
> > [SNIP]
> > 
> > So yeah, my conclusion is: destroyers are not a problem to this proposal.
> 
> I don't how this is related to the issue.

The issue was "Blitting the rvalue on top of OuterStruct.inner would not be right in that case." because "OuterStruct.inner has a destructor".

I'm saying its not a problem because it could be perfectly legal to consider that "OuterStruct.inner" has not yet been initialized.

I'm further showing this is correct behavior, as it is exactly how struct without constructors but with nested types that have destructors deal do it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------