[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