[phobos] More tuple algorithms with std.meta
Philippe Sigaud
philippe.sigaud at gmail.com
Sun Oct 3 14:38:34 PDT 2010
OK, Shin, I'm due a review of your code. Here it is:
As a general note, I find it quite elegant, even though I found your
constant use of a subdivide-and-recurse scheme a bit strange.
It makes for some clean implementations, but also complicates matters
in some cases. I'll indicate when I think a standard, linear
implementation might be more readable.
Also, maybe it'd be best if you introduced helper templates first. I
believe Phobos does this.
Anyway...
l. 8: Why redefining TypeTuple as StaticTuple? I agree the name is a
bit of a misnomer,
but I'd prefer your module to use already defined templates if possible.
l. 21-40: staticMap : instead of testing for seq.length<2 and then
have a special case for == 0, I'd write
test for == 0
else test for == 1
else (the rest)
I grok Instantiate!template.With!arguments, but really in this case
and many other, you just use a subcase.
Why not do directly map!(seq[0])?
Also, mapping is inherently linear n my mind, so using a
dividing&recurse algorithm here seems strange.
The same for filtering.
l. 55: why seq and not seq[0]? Because you want the result to always be a tuple?
l.93 (staticReduce).Ideally, there should be a 2-args version that
takes seq[0] as a seed.
l. 117: bindFront OK. Good one!
l. 143: what is m? Is that the predicate's arity? That can be
determined, given a template's symbol.
So that could be done automatically here.
l. 156: I suggest: enum size_t index = 0;
l. 210: maybe you should test for pred being a binary template (arity == 2)
l. 302 (staticCountIf): another example of recursing being strange to my eye.
Cannot it just be staticFilter!(pred, seq).length?
l. 304-316: I suggest to reorganize the code with
* length == 0
* length == 1
* else
l. 352 (_staticReplace(tr...): what if tr.length is not 2? Why not
just do _staticReplace(from,to)?
l. 354-355: what's the usefulness of Identity in this case? Cannot
just "alias tr[0] from;" work?
l. 377 (staticMost): it's a kind of fold, so it's implementable with
staticReduce. Maybe staticReduce!(selectWith!comp, seq)
with
template selectWith(comp)
{
template selectWith(A,B)
{
static if (comp!(A,B) )
alias A selectWith;
else
alias B selctWith;
}
}
or something like that, anyway.
l. 384 & 388: why Identity?
l. 414 (merge sort on tuples at CT): Nice one!
l. 422 & 430, 432 & 437: Does the compiler know what template to
instantiate for an empty tuple? I thought that was considered
ambiguous?
l. 439-448: I suggest to invert B and A in the test. Mostly, you want
to do comp!(A[0],B[0]).
And then do the then branch with A, the else branch with B. Looks more
natural to me.
l. 453: No, no ditto.
l. 498 & 506, 508 & 513: No ambiguity for an empty tuple?
l. 534: No, no ditto.
l. 565: <2 might be better, to be consistent.
l. 593: What's the difference with staticUniq?
l. 595 here also, choose <=1 or <2, but stick with it for the entire module.
l. 636-655: You know, I'd suggest a linear approach here. It'd be more
readable and probaby reduce the
number of lines.
l. 664: why do you have staticStride return 1-element tuples instead
of directly the elements?
l. 679-681: for me, another example of the dividing/recursing algo
complicating things greatly.
You even have to introduce a helper template _strideMid.
Why not just:
alias StaticTuple!(seq[0], staticStride!(n, seq[n .. $])) staticStride;
l. 699: you should test for .Expand being legal. Or maybe introduce a
isTupleOfTuples template.
Maybe implement staticTransverse with staticMap.
As a general comment on the tuples-of-tuples code: there should be a
way to segment a tuple into a tuple of n-elements tuples.
l. 732: staticZip: good idea! I never had the energy to code this.
staticZipWith would be cool.
l. 763: staticPermutations: do you have a use case in mind?
l. 765: (5 is the limit). Really? 120 5-tuplets is the limit?
l. 781 & 787: I believe metaArray is not defined in the module.
l. 879 / 884: same ambiguity question. Nice code, btw.
l. 928-929: Once more, I don't find subdividing like this the most
obvious way to code iota
Having to introduce _iotaMid feels strange.
l. 1000. Wrap is good, but I'd prefer not to introduce another
Tuple-like template.
I'd suggest using std.typecons.Tuple and extend it when possible. When
that's not possible
(as Tuple is a type and not a 'pure' template), use a free template.
For example template insertFront(A, Tuple)
l. 1113: Very clever, forcing the compiler to generates the two inner
templates to compare the arguments.
That's for being able to test aliases too, right?
l. 1271 & 1280 (templateAnd, templateOr): These are quite useful, good
idea to add them.
I have the basis of a 'type pattern' module, to look for patterns in
tuples, like for regexes. Maybe that could interest someone.
alias Either!(int, OneOrMore!double, And!(string, double)) Pattern; //
for example
l. 1295 (templateCompose): Oh yes.
Good job!
Btw, I tested Robert's suggestion to put things in a struct called
meta, and it worked:
module metaprogramming;
struct meta
{
template map { ... }
}
*-----------*
module foo;
import metaprogramming;
void main()
{
alias meta.map!(isIntegral, int, double, string, byte) Result;
writeln([Result]);
}
I find meta.* much cleaner than static* and such.
Executive Summary:
- Very nice code, very clever, lots of interesting things in there.
Maybe sometimes too clever for a standard library, that should *also*
be
a way to learn the language.
- Try to use some already-defined parts of Phobos.
- Use a linear iteration scheme for some cases: dividing and recursing
is not a panacea.
- Maybe eliminate some 'less useful' templates, like
staticPermutations unless you have a convincing use case.
Philippe
More information about the phobos
mailing list