Hello everyone,

I am currently learning D by coding a project, but I encountered
a problem with one of my structures. I managed to reduce the code
to the minimum:




import std.stdio;
import std.container.rbtree;

struct Container {
	
	private RedBlackTree!int _rbtree = new RedBlackTree!int;

	void add (int elt) {
		_rbtree.insert(elt);
	}

	void print () {
		if (_rbtree.empty) {
			writeln("empty");
		} else {
			foreach (l; _rbtree) {
				write(l, " ");
			}
			writeln();
		}
	}

}

int main () {
	Container c1, c2;
	c1.add(1);
	c1.print();
	c2.print();
	return 0;
}




I don't understand why the output of this program is:

1
1


I expect it to be:

1
empty


I thank you in advance for your help!

On 01/30/2015 11:59 AM, chardetm wrote:

> struct Container {
>
>      private RedBlackTree!int _rbtree = new RedBlackTree!int;

I think you are expecting the new expression to be be executed for every object individually. It is not the case: That new expression determines the initial value of the _rbtree for every single object of type Container. As a result, they will all be sharing the same tree.

The best solution is

1) Remove the new expression:

    private RedBlackTree!int _rbtree;

2) Use a static opCall:

    private RedBlackTree!int _rbtree;

    static Container opCall()
    {
        Container container;
        container._rbtree = new RedBlackTree!int;
        return container;
    }

3) And then initialize the variables with the following syntax:

    auto c1 = Container();
    auto c2 = Container();

Importantly, do not use the following syntax:

    Container c1;    // Warning: c1._rbttree is null

So the safe solution is (used to be?) to disable the default constructor:

    @disable this();

However, the compilation fails with the following error message:

Error: struct deneme.Container static opCall is hidden by constructors and can never be called

That looks like a bug to me because I am explicitly disabling the default constructor. Right?

In any case, an alternative solution is not to use opCall() but do what the first part of the second part of the same error message recommends:

Please use a factory method instead, or replace all constructors with static opCall.

Yes, you can have factory method that returns a Container object. The whole program with that second method:

import std.stdio;
import std.container.rbtree;

struct Container {
    private RedBlackTree!int _rbtree;

    @disable this();               // <-- disabled

    this(RedBlackTree!int rbtree)  // <-- good practice
                                   // ("parameterize from above")
    {
        _rbtree = rbtree;
    }

    void add (int elt) {
        _rbtree.insert(elt);
    }

    void print () {
        if (_rbtree.empty) {
            writeln("empty");
        } else {
            foreach (l; _rbtree) {
                write(l, " ");
            }
            writeln();
        }
    }

}

Container makeContainer()          // <-- factory method
{
    return Container(new RedBlackTree!int);
}

int main () {
    auto c1 = makeContainer();
    auto c2 = makeContainer();

    c1.add(1);
    c1.print();
    c2.print();
    return 0;
}

Ali

Thanks a lot Ali, now it works perfectly!

It is quite hard to get used to D's logic, I have to stop thinking in terms of C++...

Anyway, thanks again!

On 1/30/15 5:28 PM, Ali Çehreli wrote:
> On 01/30/2015 11:59 AM, chardetm wrote:
>
>  > struct Container {
>  >
>  >      private RedBlackTree!int _rbtree = new RedBlackTree!int;
>
> I think you are expecting the new expression to be be executed for every
> object individually. It is not the case: That new expression determines
> the initial value of the _rbtree for every single object of type
> Container. As a result, they will all be sharing the same tree.
>
> The best solution is
>
> 1) Remove the new expression:
> 2) Use a static opCall:

Why not use this() ?

On 01/30/2015 01:28 PM, Ary Borenszweig wrote:

> On 1/30/15 5:28 PM, Ali Çehreli wrote:
>> On 01/30/2015 11:59 AM, chardetm wrote:
>>
>>  > struct Container {
>>  >
>>  >      private RedBlackTree!int _rbtree = new RedBlackTree!int;
>>
>> I think you are expecting the new expression to be be executed for every
>> object individually. It is not the case: That new expression determines
>> the initial value of the _rbtree for every single object of type
>> Container. As a result, they will all be sharing the same tree.
>>
>> The best solution is
>>
>> 1) Remove the new expression:
>> 2) Use a static opCall:
>
> Why not use this() ?

In fact, I think a better solution is to use a constructor that takes the tree:

    this(RedBlackTree!int rbtree)  // <-- good practice
                                   // ("parameterize from above")
    {
        _rbtree = rbtree;
    }

However, I thought that OP did not want the users know about that member. So, as you say, the next option that comes to mind is to use the default constructor:

    this()
    {
        _rbtree = new RedBlackTree!int;
    }

Unfortunately, D does not allow defining the default constructor for structs:

Error: constructor deneme.Container.this default constructor for structs only allowed with @disable and no body

The reason is, the default constructor happens to be the .init value of that type and it must be known at compile time.

Ali

On 1/30/15 7:29 PM, Ali Çehreli wrote:
> On 01/30/2015 01:28 PM, Ary Borenszweig wrote:
>
>  > On 1/30/15 5:28 PM, Ali Çehreli wrote:
>  >> On 01/30/2015 11:59 AM, chardetm wrote:
>  >>
>  >>  > struct Container {
>  >>  >
>  >>  >      private RedBlackTree!int _rbtree = new RedBlackTree!int;
>  >>
>  >> I think you are expecting the new expression to be be executed for
> every
>  >> object individually. It is not the case: That new expression determines
>  >> the initial value of the _rbtree for every single object of type
>  >> Container. As a result, they will all be sharing the same tree.
>  >>
>  >> The best solution is
>  >>
>  >> 1) Remove the new expression:
>  >> 2) Use a static opCall:
>  >
>  > Why not use this() ?
>
> In fact, I think a better solution is to use a constructor that takes
> the tree:
>
>      this(RedBlackTree!int rbtree)  // <-- good practice
>                                     // ("parameterize from above")
>      {
>          _rbtree = rbtree;
>      }
>
> However, I thought that OP did not want the users know about that
> member. So, as you say, the next option that comes to mind is to use the
> default constructor:
>
>      this()
>      {
>          _rbtree = new RedBlackTree!int;
>      }
>
> Unfortunately, D does not allow defining the default constructor for
> structs:
>
> Error: constructor deneme.Container.this default constructor for structs
> only allowed with @disable and no body
>
> The reason is, the default constructor happens to be the .init value of
> that type and it must be known at compile time.
>
> Ali
>

Thanks for explanation. I was sure there was some reason why you didn't suggest it. It's an unfortunate inconsistency, I think. I don't know why `.init` is so important or why the default value of a type has any importance at all.