Bug 6256 – [patch] std.algorithm.map does not support static arrays and has 'length' for narrow strings.
Status
RESOLVED
Resolution
FIXED
Severity
normal
Priority
P2
Component
phobos
Product
D
Version
D2
Platform
Other
OS
Windows
Creation time
2011-07-05T17:12:00Z
Last change time
2014-12-20T13:13:14Z
Keywords
patch
Assigned to
nobody
Creator
sandford
Comments
Comment #0 by sandford — 2011-07-05T17:12:18Z
std.algorithm.map doesn't support static arrays. Two minor changes to map, and one to std.functional.unaryFunImpl (listed below) were required to make this work.
- auto map(Range)(Range r) if (isInputRange!(Unqual!Range))
+ auto map(Range)(auto ref Range r)
+ if (isInputRange!(Unqual!Range) || isArray!Range )
- alias Unqual!Range R;
+ static if(is(Unqual!Range E:E[]) ) {
+ alias E[] R;
+ } else {
+ alias Unqual!Range R;
+ }
also, map adds a length member for narrow strings. This can be fixed by changing the appropriate static if statement.
- static if (hasLength!R || isSomeString!R)
+ static if (hasLength!R)
Also, I found that a patch to unaryFunImpl similar to the patch in Issue 5406 for binaryFunImpl was needed to compile a simple test case:
void main(string[] args) {
uint[5] testing = [1,2,3,4,5];
writeln( map!"a"("testing") );
return;
}
correctly. That patch is below:
template unaryFunImpl(alias fun, bool byRef, string parmName = "a")
{
static if (is(typeof(fun) : string)) {
enum testAsExp = "{ ElementType "~parmName ~"; return ("~fun~"); }()";
static if (byRef) {
typeof(mixin(testAsExp))
result(ElementType)(ref ElementType __a)
if(__traits(compiles, mixin(testAsExp)))
{
mixin("alias __a "~parmName~";");
mixin("return (" ~ fun ~ ");");
}
} else {
typeof(mixin(testAsExp))
result(ElementType)(ElementType __a)
if(__traits(compiles, mixin(testAsExp)))
{
mixin("alias __a "~parmName~";");
mixin("return (" ~ fun ~ ");");
}
}
} else {
alias fun result;
}
}
Comment #1 by hsteoh — 2013-08-19T20:01:49Z
FWIW, map has been fixed in git HEAD to not export length with narrow strings:
import std.algorithm, std.range;
void main() {
string narrow = "abcdef";
auto m = map!(a => a)(narrow);
static assert(!hasLength!(typeof(m)));
}
Comment #2 by hsteoh — 2013-08-19T20:03:36Z
As for iterating over static arrays, one workaround is to slice it:
import std.algorithm, std.range, std.stdio;
void main() {
uint[4] test = [1,2,3,4];
writeln(map!(a=>a+10)(test[]));
}
Comment #3 by hsteoh — 2014-12-05T23:05:52Z
The length issue has already been fixed.
As for static arrays, I think the consensus on github is that static arrays are not ranges; if you want to use an algorithm on a static array, use [] to slice it first (akin to using [] on std.container containers to get a range out of them -- containers are not ranges).
So, I'm closing this bug as fixed.
Comment #4 by bearophile_hugs — 2014-12-06T00:06:14Z
(In reply to hsteoh from comment #3)
> The length issue has already been fixed.
>
> As for static arrays, I think the consensus on github is that static arrays
> are not ranges; if you want to use an algorithm on a static array, use [] to
> slice it first (akin to using [] on std.container containers to get a range
> out of them -- containers are not ranges).
Slicing a fixed size array works, but it throws away an extremely useful piece of information, namely the compile-time knowledge of their length. This is awful for small arrays, because it kills the simple possibilities of the compiler to unroll loops and perform other optimizations. You have a language with a strong type system that keeps the compile-time length of some arrays, and then you throw it away, this is very bad. Phobos will need to take much more in account the presence of fixed-size arrays if it wants to be an efficient library.
What I'd like is to map to call a specialized map function if you call it on a _small_ fixed-size array, and such map can call the regular map function with longer fixed-size arrays. This avoids template bloat and helps the back-end optimize for small loops. I guess it's better to open a new enhancement request for this?
Comment #5 by hsteoh — 2014-12-06T01:31:33Z
Sure, open another enhancement request for this.
Loop-unrolling is already done by GDC (and probably LDC) anyway, so it's not clear that having many copies of a function, each with a different static array length but otherwise identical, which is a lot of template bloat, would give any clear advantages over a single, general purpose function that takes length as a runtime parameter. Advanced loop unrolling, of the kind that I've observed gdc do, can do things like unrolling a loop into blocks of n iterations, then if the incoming array length is exactly n, it just runs through the entire block with no conditionals until the end, or if the length is < n, it branches to the middle of the block so that no conditionals are evaluated until the end. This reduces pressure on the CPU cache (no risk of the entire code page containing the function being filled with other instantiations of the same function with other static array lengths, which means an RAM roundtrip between calling/exiting the function, when the single function could have fit on the same page as its caller, thus eliminating a RAM roundtrip).
In any case, the picture is complicated by the complexity of modern hardware, so it's hard to say one way or another which approach will give better performance. If you have many different-sized static arrays in your program, the template bloat can become quite horrendous (e.g., if you have static arrays of all lengths from 1 to 10, and all of them were unrolled, you'd have 55 copies of the loop body), whereas an advanced loop unroller could accomplish basically the same thing plus just 1 branch, and without any of the template bloat (at most 10 copies of the loop body would be necessary to ensure no conditionals for all loops over the arrays until the end). Exactly where to draw the line is quite machine-dependent, and I would rather let the optimizer do its job instead of trying to force template instantiations that may not wind up actually doing any better.
Comment #6 by issues.dlang — 2014-12-07T12:19:30Z
If you want to argue for a special overload for static arrays, please open a new enhancement request. However, in general, I think that supporting static arrays with range-based functions is just asking for trouble, because they're not ranges (e.g. popFront does not and cannot work for them). So, I think that benchmarks which show a significant improvement with static arrays being special-cased would be required for it to be worth considering.
Comment #7 by bearophile_hugs — 2014-12-07T23:34:34Z
(In reply to Jonathan M Davis from comment #6)
> So, I think that benchmarks which show a significant improvement with static
> arrays being special-cased would be required for it to be worth considering.
I agree.
Comment #8 by bearophile_hugs — 2014-12-20T13:13:14Z
(In reply to bearophile_hugs from comment #7)
> (In reply to Jonathan M Davis from comment #6)
> > So, I think that benchmarks which show a significant improvement with static
> > arrays being special-cased would be required for it to be worth considering.
>
> I agree.
I don't have done benchmarks yet, but I have added another Enhancement Request for Phobos, see Issue 13880