www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 9732] New: Do not call opAssign() for the first assignment to member in the constructor

reply d-bugmail puremagic.com writes:
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



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: -------
Mar 15 2013
next sibling parent d-bugmail puremagic.com writes:
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



---
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: -------
Mar 15 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732




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: -------
Mar 16 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732


monarchdodra gmail.com changed:

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





 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732






 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732




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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732




---




 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732




---


 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732










 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732








 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: -------
Mar 17 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732




---
Another problem can arise from mixin usage - when you write mixin or mixin
template you assume that opAssign would be called, but this assumptions breaks
and there is no opportunities to detect it.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Mar 18 2013
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=9732


Kenji Hara <k.hara.pg gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |DUPLICATE



---
*** This issue has been marked as a duplicate of issue 9665 ***

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