Deque impl.
Robert Schadek
realburner at gmx.de
Tue Jan 29 14:44:22 PST 2013
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
More information about the Digitalmars-d
mailing list