Skip to content

Comments

Fix qualified constructor call semantics.#1726

Merged
WalterBright merged 5 commits intodlang:masterfrom
9rnsr:fix_qualified_ctor
Apr 19, 2013
Merged

Fix qualified constructor call semantics.#1726
WalterBright merged 5 commits intodlang:masterfrom
9rnsr:fix_qualified_ctor

Conversation

@9rnsr
Copy link
Contributor

@9rnsr 9rnsr commented Mar 7, 2013

By this change, one of following is required for immutable object construction:

  1. immutable constructor
    That is described in TDPL.

    struct S {
      int[] value;
      this(int v, int n) immutable { foreach (_; 0 .. n) value ~= v; }
    }
    void main() {
      auto i = immutable S(1, 3); // OK
      assert(i.value == [1,1,1]);
    
      auto c = const S(1, 3); // also OK
      // because immutable object is implicitly convertible to const
    
      static assert(!__traits(compiles, S(1, 3))); // not allowed
    }
  2. inout constructor
    The qualifier of generated object by inout constructor is calculated as if that is returned value from inout function.

    struct S {
      int[] value;
      this(inout int[] arr) inout { value = arr; }
    }
    void main() {
      auto m = S([1,2,3]); // OK
      auto c = const S([1,2,3]); // OK
      auto i = immutable S([1,2,3].idup); // OK
    
      static assert(!__traits(compiles, immutable S([1,2,3]))); // not allowed
      static assert(!__traits(compiles, S([1,2,3].idup))); // not allowed
    }
  3. unique constructor
    If a pure constructor can create unique object, the generated object can be implicitly convertible to any qualifier.

    struct S {
      int[] value;
      this(inout int[] arr) pure {
        value = arr.dup;
      }
    }
    void main() {
      auto m = S([1,2,3]); // OK
      auto c = const S([1,2,3]); // OK
      auto i = immutable S([1,2,3].idup); // OK
    
      auto m2 = S([1,2,3].idup);   // also OK
      auto i2 = immutable S([1,2,3]);  // also OK
    
      auto s = shared S([1,2,3]); // OK!
      auto sc = shared S([1,2,3]); // OK!
    }

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 7, 2013

Eh, sorry @yebblies, I found that the devil's patch (that was in #666) is actually necessary.

@yebblies
Copy link
Contributor

yebblies commented Mar 7, 2013

Could you please add regression tests for the doHeaderInstantiation changes (or point me to them if I missed them)?

If I have

struct S
{
    this() immutable {}
}

and I try to create it like this it fails, right?

auto s = S();

But this would work.

S s = immutable(S)();

Is this right, and could we remove this limitation?

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 7, 2013

Could you please add regression tests for the doHeaderInstantiation changes (or point me to them if I missed them)?

They have no bugzilla number, but the last commit implicitly contains the tests for the bug (that is test15[abc])).

struct S { this() immutable {} }

That is not valid D code, because no-arg constructor is not allowed.

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 7, 2013

With struct S { this(int n) immutable {} }

auto s = S(1);
S s = immutable(S)(1);

Essentially both should be rejected (but this change does not reject the latter now)

Because. S s = immutable(S)(1); is lowered to:

S s = void;
s.__ctor(1);  // __ctor == this(int n) immutable

(Note that, D optimizes constructor call in initialization. It is different from C++)
But the lowered code would break const correctness.

@yebblies
Copy link
Contributor

yebblies commented Mar 7, 2013

They have no bugzilla number, but the last commit implicitly contains the tests for the bug (that is test15[abc])).

Ah right, I mistakenly assumed that doHeaderInstantiation had something to do with di generation.

struct S { this() immutable {} }

That is not valid D code, because no-arg constructor is not allowed.

Right, I meant a valid constructor with an argument.

Essentially both should be rejected (but this change does not reject the latter now)

Because. S s = immutable(S)(1); is lowered to:

S s = void;
s.__ctor(1);  // __ctor == this(int n) immutable

Is this part of the language though, or just dmd's implementation? I see no reason it couldn't be lowered to

S s = void;
immutable(S) tmp;
tmp.__ctor(1);
s = tmp;

I know that's not valid code, but hopefully you get what I mean.

One of the goals of making struct literals rvalues was to make S(args) and () => S(args) more similar, and this seems like a step in the other direction, as return values will implicitly convert, but direct construction will not.

(I'm talking about the S s = immutable(S)(1); syntax, not the other one)

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 7, 2013

Is this part of the language though, or just dmd's implementation? I see no reason it couldn't be lowered to ...

(I'm talking about the S s = immutable(S)(1); syntax, not the other one)

I can agree that your thought is not invalid, but I can see no benefit in there.

One of the goals of making struct literals rvalues was to make S(args) and () => S(args) more similar, and this seems like a step in the other direction, as return values will implicitly convert, but direct construction will not.

No, a constructor call in variable initializer is treated specially in today's D.

In C++, for S s = S(1); the language specification requires redundant copying (after unnamd object constructed, copy it to variable s). I have thought that is one of human unfriendly feature because there is hidden copy cost.
In D, it is improved. The constructor call in initializer is always optimized and there is no copying. But your idea will drag the bad feature from the grave. If S has high-cost postblit constructor, using that syntax will suddenly make execution speed slower. I think we should not allow such debug-unfriendly feature.

@yebblies
Copy link
Contributor

yebblies commented Mar 7, 2013

In D, it is improved. The constructor call in initializer is always optimized and there is no copying.

Is this required or is this just an optimization made possible by D's rvalue semantics?

If S has high-cost postblit constructor

(Is the implicit conversion from/to mutable/immutable even valid if S has a postblit? Have we finished defining this part of the language?)

I understand where you're coming from, but you could also say the bad performance is the fault of the high-cost postblit itself, not the language.

I think this situation is inconsistent:

struct S
{
    this(int i) immutable {}
    this(this) { beSlow(); }
    ref S opAssign(const ref S other) const { beSlow(); }
}
immutable(S) makeS() { return immutable(S)(0); }
void main()
{
    immutable(S) s1 = makeS(); // fast, rvalue copy
    immutable(S) s2 = immutable(S)(0); // fast, in-place construction
    S s3 = makeS(); // slow, postblit called (or is it opAssign?)
    S s4 = immutable(S)(0); // not allowed????
}

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 7, 2013

Is this required or is this just an optimization made possible by D's rvalue semantics?

Oh, I forgot to mention about that in previous comment. At least it has been enabled from early D2 age (See VarDeclaration::semantic). Indeed that might not be properly documented, but I can believe that @WaltarBright has been intended it as one improvement point of D from C++

(Is the implicit conversion from/to mutable/immutable even valid if S has a postblit? Have we finished defining this part of the language?)

I'm planning it in local (unique postblit would be introduced). I should write a new DIP for that.

I understand where you're coming from, but you could also say the bad performance is the fault of the high-cost postblit itself, not the language.

No, I mean is that postblit (copying) should not run for object construction. If the variable declaration looks like being initialized by constructor call, it should actually not do copying. I think your proposal will destroy the implicit assumption of many D users.

I think this situation is inconsistent:

All of them are initializing, not assignment. The last one looks to me that is just a mistaken.

@yebblies
Copy link
Contributor

yebblies commented Mar 8, 2013

Oh, I forgot to mention about that in previous comment. At least it has been enabled from early D2 age (See VarDeclaration::semantic). Indeed that might not be properly documented, but I can believe that @WaltarBright has been intended it as one improvement point of D from C++

I recall it being carefully designed to be possible to avoid copies on construction, but I don't know if it is required. If it is then we don't really have a choice here.

I'm planning it in local (unique postblit would be introduced). I should write a new DIP for that.

Looking forward to it!

No, I mean is that postblit (copying) should not run for object construction. If the variable declaration looks like being initialized by constructor call, it should actually not do copying. I think your proposal will destroy the implicit assumption of many D users.

All of them are initializing, not assignment. The last one looks to me that is just a mistaken.

I'll have to think more about this.

@andralex
Copy link
Member

This is great work. I'm not very sure about the pure constructor; I know there's been some talk about it, are @9rnsr and @WalterBright and others certain this stands scrutiny?

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 11, 2013

@andralex "unique constructor" is a concept that I call tentatively.

Some kind of pure function can return unique object that has no indirection to existing objects.
The concept had proposed in issue 5081 and implemented by @yebblies (#218), and recently I reinforced it in #1110 and #1519.

By using the concept, some pure constructors can construct unique object, and compiler can implicitly convert the generated objects to any qualifier. So I call them "unique constructor".

That is incredibly useful concept, and I think that we also can improve postblit concept by using it. (introducing "unique postlit" and use it for mutual conversion between mutable and immutable objects. I planning to write new DIP about it.)

@yebblies
Copy link
Contributor

@andralex I assume you meant me? I'm happy with the purity related changes in this pull. I think they're in the right direction.
I'm more concerned about the differences between constructor construction and function construction, as I've said in my other comments. An official answer from you and/or @WalterBright would be helpful here.

@9rnsr
Copy link
Contributor Author

9rnsr commented Mar 13, 2013

I think we need to add this in 2.063. In 2.063, bug 6578 was partially fixed (by #666), but by that, currently incorrect constructor is easy to be called.

struct S {
    int[] arr;
    this(int[] a) { arr = a; }
}
void main() {
    int[] arr = [1,2,3];
    auto si = immutable S(arr);
    pragma(msg, typeof(si));
    pragma(msg, typeof(si.arr));
    assert(si.arr == [1,2,3]);
    arr[] = 1;
    assert(si.arr == [1,1,1]);  // !?
}

This code compiles and runs successfully with current master. That's really bad.

@9rnsr
Copy link
Contributor Author

9rnsr commented Apr 13, 2013

Temporarily close.

@9rnsr 9rnsr closed this Apr 13, 2013
@9rnsr 9rnsr reopened this Apr 16, 2013
9rnsr added 5 commits April 16, 2013 13:16
Confused `this(this)` and `this(this T)`
If the qualified constructor creates isolated object, it is implicitly convertible to any qualified object.

And now, inout constructor is properly supported.
@WalterBright
Copy link
Member

I'll pull this for now. But I want to work on developing the idea of a "unique constructor". A constructor shouldn't have to be labeled 'pure' in order for it to be unique. It can be inferred to be unique if it consists solely of unique (or immutable) elements.

The result of a unique constructor can be implicitly cast to mutable, immutable, or shared.

WalterBright added a commit that referenced this pull request Apr 19, 2013
Fix qualified constructor call semantics.
@WalterBright WalterBright merged commit f27a047 into dlang:master Apr 19, 2013
@9rnsr 9rnsr deleted the fix_qualified_ctor branch April 19, 2013 16:22
@schveiguy
Copy link
Member

FYI, a bug I opened was closed due to this pull, but I don't agree with the solution (for inout anyway).

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

Please comment...

@CyberShadow
Copy link
Member

This pull request introduced a regression:
https://issues.dlang.org/show_bug.cgi?id=14351

wilzbach pushed a commit to wilzbach/dmd that referenced this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants