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
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
//----
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.
3. Nitpick: Don't use parenthesis for single arg templates:
No: "Deque!(int)"
yes: "Deque!int"
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...
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'll review the actual deque implementation tomorrow.
