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
