Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

static array litteral: [1,2].asStatic + overloads for CT ranges, RT ranges, explicit types #6178

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions changelog/std-array-asStatic.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
Added `staticArray` to construct a static array from input array / range / CT range.

The type of elements can be specified implicitly (`[1,2].staticArray` of type int[2])
or explicitly (`[1,2].staticArray!float` of type float[2]).
When `a` is a range (not known at compile time), the number of elements has to be given as template argument
(eg `myrange.staticArray!2`).
Size and type can be combined (eg: `2.iota.staticArray!(byte[2])`).
When the range `a` is known at compile time, it can also be specified as a
template argument to avoid having to specify the number of elements
(eg: `staticArray!(2.iota)` or `staticArray!(double, 2.iota)`).

Note: `foo([1, 2, 3].staticArray)` may be inefficient because of the copies involved.

---
auto a1 = [0, 1].staticArray;
static assert(is(typeof(a1) == int[2]));
assert(a1 == [0, 1]);

auto a2 = [0, 1].staticArray!byte;
static assert(is(typeof(a2) == byte[2]));

import std.range : iota;
auto input = 2.iota;
auto a3 = input.staticArray!2;
auto a4 = input.staticArray!(byte[2]);

auto a5 = staticArray!(2.iota);
auto a6 = staticArray!(double, 2.iota);
---
215 changes: 215 additions & 0 deletions std/array.d
Original file line number Diff line number Diff line change
Expand Up @@ -3927,3 +3927,218 @@ unittest
assert(appS.data == "hellow");
assert(appA.data == "hellow");
}

/++
Constructs a static array from `a`.
The type of elements can be specified implicitly (`[1,2].staticArray` of type int[2])
or explicitly (`[1,2].staticArray!float` of type float[2]).
When `a` is a range (not known at compile time), the number of elements has to be given as template argument
(eg `myrange.staticArray!2`).
Size and type can be combined (eg: `2.iota.staticArray!(byte[2])`).
When the range `a` is known at compile time, it can also be specified as a
template argument to avoid having to specify the number of elements
(eg: `staticArray!(2.iota)` or `staticArray!(double, 2.iota)`).

Note: `staticArray` returns by value, so expressions involving large arrays may be inefficient.

Params: a = The input elements

Returns: A static array constructed from `a`.
+/

pragma(inline, true) T[n] staticArray(T, size_t n)(auto ref T[n] a) nothrow @safe pure @nogc
{
return a;
}

/// ditto
U[n] staticArray(U, T, size_t n)(auto ref T[n] a) nothrow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd accept the argument "let's leave it nothrow and worry about it later) if (a) this PR were urgent, and/or (b) the right code would be difficult to implement. Neither is applicable. The right code is (stylized):

U[n] staticArray(U, T, size_t n)(auto ref T[n] a)
{
    union Temp { U[n] payload; }
    Temp result = void;
    size_t i = 0;
    static if (hasElaborateDestructor!U) scope (exit)
    {
        foreach (j; 0 .. i)
            destroy(result.payload[j]);
    }
    for (; i != n; ++i)
        emplaceRef!U(result.payload[i], cast(U) a[i]);
    return result.payload;
}

That's awkward and disables named return value optimization, so here's what I think would be a better approach:

U[n] staticArray(U, T, size_t n)(auto ref T[n] a)
{
    U[n] result = void;
    size_t i = 0;
    static if (hasElaborateDestructor!U) scope (failure)
    {
        foreach (j; i .. n)
            emplace(result[j]); // initialize with T.init to make sure is destroyable
    }
    for (; i != n; ++i)
        emplaceRef!U(result[i], cast(U) a[i]);
    return result;
}

This is cute and hopefully not too much so - it comes from the opposite direction: in case of an exception, it initializes (in a nothrow manner) the uninitialized part of the result and then leaves the rest to the compiler. This allows NRVO, is efficient on the nothrow path, and is simple.

if (!is(U == T))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template constraint should include that T can be cast to U.

{
import std.conv : emplaceRef;

U[n] ret = void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has a pernicious bug: if construction throws an exception, ret will be destroyed and destructors will be called for uninitialized objects. Recall that statically-sized arrays on the stack insert one destructor call for each member.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoided this bug by making these overloads as nothrow, so a throwing ctor will cause CT error instead of potential runtime bug.

support for throwing constructors can always be addressed in a future PR (without breaking backward compatibility), maybe something along these lines:

U[n] staticArray(U, T, size_t n)(auto ref T[n] a)
if (!is(U == T))
{
     import std.conv : emplaceRef;

     U[n] ret = void;
    size_t i;
    try
     {
        for (; i < n; i++)
        {
            emplaceRef!U(ret[i], cast(U) a[i]);
        }
    }
    catch(Exception t) // catch(Throwable t) causes errors with D linter; but we really want Throwable here for correctness
    {
        for (; i < n; i++)
        {
            emplaceRef!U(ret[i], U.init);
        }
        throw t;
     }
     return ret;
 }

// TODO: ElementType vs ForeachType
static foreach (i; 0 .. n)
{
emplaceRef!U(ret[i], cast(U) a[i]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cast here is dangerous, especially since it's virtually undocumented. I understand the necessity is to create arrays of short, float etc. from literals. But we can't afford to allow innocent syntax like [1, 2, 3].asStatic!Object to disastrously pass.

I'll "ask for changes" on account of this.

I recall you had a variant asStaticCast that took care of this. We may need to return to that, but probably we could find a better idea. We could special-case numbers at most, but not allow unchecked casts. Another possibility is to use to.

This is the kind of decision that can be deferred - I suggest you remove this overload entirely, get this in, then add something later.

}
return ret;
}

/// static array from array
nothrow pure @safe unittest
{
auto a = [0, 1].staticArray;
static assert(is(typeof(a) == int[2]));
assert(a == [0, 1]);

auto b = [0, 1].staticArray!byte;
static assert(is(typeof(b) == byte[2]));
assert(b == [0, 1]);
}

nothrow pure @safe unittest
{
int val = 3;
static immutable gold = [1, 2, 3];
[1, 2, val].staticArray.checkStaticArray!int([1, 2, 3]);

@nogc void checkNogc()
{
[1, 2, val].staticArray.checkStaticArray!int(gold);
}

checkNogc();

[1, 2, val].staticArray!double.checkStaticArray!double(gold);
[1, 2, 3].staticArray!int.checkStaticArray!int(gold);

[1, 2, 3].staticArray!(const(int)).checkStaticArray!(const(int))(gold);
[1, 2, 3].staticArray!(const(double)).checkStaticArray!(const(double))(gold);
{
const(int)[3] a2 = [1, 2, 3].staticArray;
}

[1, 129].staticArray!byte.checkStaticArray!byte([1, -127]);

}

/// ditto
auto staticArray(size_t n, T)(T a)
if (isInputRange!T)
{
alias U = ElementType!T;
return staticArray!(U[n], U, n)(a);
}

/// ditto
auto staticArray(Un : U[n], U, size_t n, T)(T a) nothrow
if (isInputRange!T)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import std.conv : emplaceRef;

U[n] ret = void;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same discussion of nothrow

size_t i;
foreach (ref ai; a)
{
emplaceRef!U(ret[i++], cast(U) ai);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFINITELY no cast here

}
assert(i == n);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely unsatisfactory. Code shouldn't rely on bounds checking to be correct (see https://dlang.org/spec/arrays.html#bounds), and by the time the assert is even looked at the code has trampled well into infested territory.

Too short is not good, either, because there'd be portions of the array that are left uninitialized! This function is thoroughly problematic.

One solution I suggested to @n8sh: define an overload taking an out size_t informing the user on how many elements have been initialized, and initialize the rest to U.init. To address the other problem (input range too long), we could probably accept truncation. After all the user only seems to be interested in a fragment of the range. They could check the length externally if they cared. All told:

auto staticArray(Un : U[n], U, size_t n, T)(T a, out size_t filled)
{
    U[n] result = void;
    size_t i = 0;
    // same exception safety mechanism as above
    foreach (ref ai; a)
    {
        if (i == n) break;
        emplace(...);
    }
    filled = i;
    return result;
}

auto staticArray(Un : U[n], U, size_t n, T)(auto ref T a)
{
    size_t unused;
    return staticArray!(Un, U, n, T)(a, unused);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to mention, some ranges return an rvalue from front so using foreach is problematic with those. It's simplest to use for.

return ret;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code duplicates implementation (and bug) from a different overload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

/// static array from range + size
nothrow pure @safe unittest
{
import std.range : iota;

auto input = 2.iota;
auto a = input.staticArray!2;
static assert(is(typeof(a) == int[2]));
assert(a == [0, 1]);
auto b = input.staticArray!(byte[2]);
static assert(is(typeof(b) == byte[2]));
assert(b == [0, 1]);
}

nothrow pure @safe unittest
{

auto a = [1, 2].staticArray;
assert(is(typeof(a) == int[2]) && a == [1, 2]);

import std.range : iota;

2.iota.staticArray!2.checkStaticArray!int([0, 1]);
2.iota.staticArray!(double[2]).checkStaticArray!double([0, 1]);
2.iota.staticArray!(byte[2]).checkStaticArray!byte([0, 1]);
}

nothrow pure @system unittest
{
import std.range : iota;

assert(isThrown!Error(2.iota.staticArray!1));
assert(isThrown!Error(2.iota.staticArray!3));

// NOTE: correctly issues a deprecation
// int[] a2 = [1, 2].staticArray;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure whether it's a good idea to include such dead blocks. Maybe open a Bugzilla issue to fix asStatic that depends on 16779 so that this isn't forgotten?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually 16779 was for VRP on staticArray(T...)(T a) which I removed (i just have asStatic now), so removed that;
also, folded other dead block into a comment in preceding unittest.

__traits(deprecated) would be the optimal (see https://dlang.slack.com/archives/D8ZMKRGUV/p1519414575000349)


/// ditto
auto staticArray(alias a)()
if (isInputRange!(typeof(a)))
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this entire block be in static immutable = () {...} to take full advantage of CTFE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced by:

auto asStatic(alias a)() nothrow @safe pure @nogc
{
    return .asStatic!(cast(size_t) a.length)(a);
}

not sure if that's what you meant but the newer form is indeed much better to avoid duplication

// NOTE: without `cast`, getting error:
// cannot deduce function from argument types !(length)(Result)
return .staticArray!(cast(size_t) a.length)(a);
}

/// ditto
auto staticArray(U, alias a)()
if (isInputRange!(typeof(a)))
{
return .staticArray!(U[cast(size_t) a.length])(a);
}

/// static array from CT range
nothrow pure @safe unittest
{
import std.range : iota;

enum a = staticArray!(2.iota);
static assert(is(typeof(a) == int[2]));
assert(a == [0, 1]);

enum b = staticArray!(byte, 2.iota);
static assert(is(typeof(b) == byte[2]));
assert(b == [0, 1]);
}

nothrow pure @safe unittest
{
import std.range : iota;

enum a = staticArray!(2.iota);
staticArray!(2.iota).checkStaticArray!int([0, 1]);
staticArray!(double, 2.iota).checkStaticArray!double([0, 1]);
staticArray!(byte, 2.iota).checkStaticArray!byte([0, 1]);
}

version(unittest) private void checkStaticArray(T, T1, T2)(T1 a, T2 b) nothrow @safe pure @nogc
{
static assert(is(T1 == T[T1.length]));
assert(a == b);
}

/+
TODO: consider adding this to assertThrown in std.exception
The alternatives are not nothrow (see https://issues.dlang.org/show_bug.cgi?id=12647)
---
import std.exception : assertThrown;
assertThrown!Error(expr_that_throws);

// or
import std.exception : ifThrown;
scope(failure) assert(0);
assert(ifThrown!Error({ expr_that_throws; return false; }(), true));
---
Also, `isThrown` is easier to use than ifThrown and more flexible than assertThrown.
+/
version(unittest) private bool isThrown(T : Throwable = Exception, E)(lazy E expression) nothrow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

{
try
{
expression();
return false;
}
catch (T)
{
return true;
}
catch (Exception)
{
return false;
}
}