Deque impl.

[Robert Schadek]

On 01/29/2013 09:20 PM, monarch_dodra wrote:
> On Tuesday, 29 January 2013 at 19:44:23 UTC, Robert Schadek wrote:
>> Hi,
>>
>> I have a Deque implementation that I really like. I would like to get
>> some comments on it.
>> http://dpaste.1azy.net/4bf119e7 dpaste does not run the unittests
>> successfully. I don't
>> know why. I tested on a x32 as well as x64 machine and it works on both.
>>
>> Best Regards
>> Robert
>
> Appart from the fact you seem to have copy pasted your code twice, 
> here are a quick comments, interface wise. I didn't check anything 
> implementation-wise
fixed
>
> 1. When you put a unittest inside the definition of a class/struct, it 
> gets run for every instantiation of said class, and has access to "T". 
> This is good for writing generic tests for the current T type. This 
> can be useful to test things that depend on T, such as making sure 
> some functions are indeed safe, or whatnot.
>
> If you are testing a specific implementation, then move it out of the 
> struct.
>
> For example:
> //----
> import std.stdio;
>
> struct S(T)
> {
>     unittest
>     {
>         //Tests S!T for every T.
>         writeln("Unittest: S!", T.stringof);
>     }
> }
>
> unittest
> {
>     //Write a specific S!int test here, for example
>     writeln("Unittest: Global");
> }
>
> void main()
> {
>     alias A = S!int;    //Force S!int tests
>     alias B = S!double; //Force S!int tests
> }
> //----
>
> "rdmd -unittest main.d" produces:
> //----
> Unittest: Global
> Unittest: S!int
> Unittest: S!double
> //----
I see the point. I will fix that.
>
> 2. Nitpick: I'd personally prefer you use the term "Range" for your 
> iterator. Iterator != Range. You are confusing me. Also, I'd consider 
> nesting the iterator definition inside your deque.
That on my list as well.
>
> 3. Nitpick: Don't use parenthesis for single arg templates:
> No:  "Deque!(int)"
> yes: "Deque!int"
May I ask why? Both are correct after all. Or is it just the preferred 
style?
>
> 4. Why does your iterator have two template parameters? Seems it is 
> always "T" and "Deque!T". Why bother with the second parameter at all? 
> If you place it inside your Deque implementation, then it becomes 
> parameter-less (appart from the implicit T of course). The added 
> advantage is that it becomes a "Deque!int.Iterator" type. This is good 
> if you ever write "List", and don't want to have the name "Iterator" 
> clash...
I did this to return a Iterator from a const(Deque). But placing it 
nested is the better and prettier way.
>
> 5. Users expect "opSlice()" to produce an actual range. You are using 
> it to deep copy the deck. This goes against your comment in regards to 
> slicing an iterator. Just have your "opSlice()" do the same as what 
> "range()" does, and get rid of "range()". This is the usage I'd expect:
>
> //----
> Deque!int    de = ... ;
> Iterator!int it = de[];
> writeln(it);
> //----
I submitted a bug report for sort recently (9091 became 8368). 
monarchdodra said (and pointed to source) that opSlice should return the 
same type in respect to the called object.
>
> I'll review the actual deque implementation tomorrow.
Thanks