Hi,

I have a template class "List" which should provide generic list functionality. In my use case, using a class instead a struct
is more comfortable.

Following unittest fails, although the forward range
implements the save method.

unittest
{
	auto intList = new List!int(1,2,3);
	assert(intList.length == 3);
	foreach(item; intList) {}
	assert(intList.length == 3);
}


class List(T)
{
	private T[] items;
	
	this(T[] items...)
	{
		this.items = items;
	}

	bool empty() const
    {
        return items.length == 0;
    }
	
	@property int length() const
	{
		return items.length;
	}

    void popFront()
    {
        items = items[1..$];
    }
	
	@property List!T save() const
	{
		return new List!T( cast(T[]) items);
	}

	T front() const
    {
        return opIndex(0);
    }
	
	T opIndex(size_t index) const
    {
        return cast(T) items[index];
    }
}

I already tried different thinks, like using dup or importing std.array.
But nothing helps, the unittest fails.
Why it fails?

Kind regards
André

On Fri, Jun 13, 2014 at 08:14:11PM +0200, Andre via Digitalmars-d-learn wrote:
> Hi,
> 
> I have a template class "List" which should provide generic list functionality. In my use case, using a class instead a struct is more comfortable.
> 
> Following unittest fails, although the forward range implements the save method.
[...]

Generally, it's a bad idea to conflate a container with a range over its contents. In this sense, built-in arrays are a very bad example, because they show precisely this sort of conflation. But they get away with it because of their hybrid value/reference semantics. Most containers, however, don't have this sort of complex semantics, so they don't mix very well with ranges directly.

It's much better to separate the container from a range over its contents. So I wouldn't put the range API methods in the List class, but implement an opSlice method that returns a struct that performs the list iteration, that implements the range API. This allows you to write:

	auto myList = new List(...);
	foreach (item; myList[]) { // N.B., myList[] calls myList.opSlice()
		...
	}

Since the range struct returned by opSlice is separate from the container itself, you don't have any risk of iteration also consuming the container.


T

-- 
Let X be the set not defined by this sentence...

Am 13.06.2014 20:22, schrieb H. S. Teoh via Digitalmars-d-learn:
> [...]
>
> Generally, it's a bad idea to conflate a container with a range over its
> contents. In this sense, built-in arrays are a very bad example, because
> they show precisely this sort of conflation. But they get away with it
> because of their hybrid value/reference semantics. Most containers,
> however, don't have this sort of complex semantics, so they don't mix
> very well with ranges directly.
>
> It's much better to separate the container from a range over its
> contents. So I wouldn't put the range API methods in the List class, but
> implement an opSlice method that returns a struct that performs the list
> iteration, that implements the range API. This allows you to write:
>
> 	auto myList = new List(...);
> 	foreach (item; myList[]) { // N.B., myList[] calls myList.opSlice()
> 		...
> 	}
>
> Since the range struct returned by opSlice is separate from the
> container itself, you don't have any risk of iteration also consuming
> the container.
>
>
> T
>

Thank you, this information is really helpful.

Kind regards
André

On Friday, 13 June 2014 at 18:14:10 UTC, Andre wrote:
> Hi,
>
> I have a template class "List" which should provide generic list functionality. In my use case, using a class instead a struct
> is more comfortable.
>
> Following unittest fails, although the forward range
> implements the save method.
>
> unittest
> {
> 	auto intList = new List!int(1,2,3);
> 	assert(intList.length == 3);
> 	foreach(item; intList) {}
> 	assert(intList.length == 3);
> }
>
> ...
>
> I already tried different thinks, like using dup or importing std.array.
> But nothing helps, the unittest fails.
> Why it fails?
>
> Kind regards
> André

To add to what H.S. Teoh said, which is good advice, but doesn't quite explain why your unittest failed: Foreach takes a copy of your range, and then "consumes it". This may or may not have an effect on the original range. If you plan to use your range again after a call, you are supposed to *save* it:

unittest
{
	auto intList = new List!int(1,2,3);
	assert(intList.length == 3);
	foreach(item; intList.save) {} //HERE
	assert(intList.length == 3);
}

There.

I am making the following comment to have others confirm, as well as remind others about a potential problem.

On 06/13/2014 11:14 AM, Andre wrote:

> unittest
> {
>      auto intList = new List!int(1,2,3);

[...]

> class List(T)
> {
>      private T[] items;
>
>      this(T[] items...)
>      {
>          this.items = items;
>      }

Unrelated to your question, I think that is a bug because you are keeping a reference to a temporary array. The compiler will generate a temporary array for 1, 2, and 3 and then that array will disappear.

Am I remembering correctly?

If so, I would recommend against 'T[] items...' but use an explicit slice:

    this(T[] items)

The users will slightly be inconvenienced but the program will be correct.

Or, you can continue with 'T[] items...' and then have to take a copy of the argument:

        this.items = items.dup;    // copied

That is obviously slower when the user actually passed in a slice. That's why I think it is better to take an explicit slice as a parameter.

Ali

On Friday, 13 June 2014 at 19:03:12 UTC, Ali Çehreli wrote:
> I am making the following comment to have others confirm, as well as remind others about a potential problem.
>
> On 06/13/2014 11:14 AM, Andre wrote:
>
> > unittest
> > {
> >      auto intList = new List!int(1,2,3);
>
> [...]
>
> > class List(T)
> > {
> >      private T[] items;
> >
> >      this(T[] items...)
> >      {
> >          this.items = items;
> >      }
>
> Unrelated to your question, I think that is a bug because you are keeping a reference to a temporary array. The compiler will generate a temporary array for 1, 2, and 3 and then that array will disappear.
>
> Am I remembering correctly?
>
> If so, I would recommend against 'T[] items...' but use an explicit slice:
>
>     this(T[] items)
>
> The users will slightly be inconvenienced but the program will be correct.
>
> Or, you can continue with 'T[] items...' and then have to take a copy of the argument:
>
>         this.items = items.dup;    // copied
>
> That is obviously slower when the user actually passed in a slice. That's why I think it is better to take an explicit slice as a parameter.
>
> Ali

Yes.

`fun(T)(T[]...)` is just both `fun(T...)(T t)` and `fun(T)(T[] t)` both "conveniently" packaged into one, but with twice the pitfalls.

I'd suggest avoiding it altogether. It's a non-feature (IMO).