-
-
Notifications
You must be signed in to change notification settings - Fork 703
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
Conversation
Thanks for your pull request and interest in making D better, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#6178" |
There's no reason to break up std.array. I suggest you leave that change out. |
std/array/static_array.d
Outdated
pragma(inline, true) U[T.length] staticArrayCast(U, T...)(T a) @nogc | ||
{ | ||
enum n = T.length; | ||
U[n] ret = void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If U
is a struct you need to check if it has an elaborate destructor and in that case not void-initialize ret
because the destructor for ret[i]
will be run before a[i]
is blitted over it. An alternative could be to use emplace
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, used emplaceRef wherever applicable
added motivation for it in intro: std.array is a large package and PTAL, I've improved API to allow all cases (see updated top-level msg) |
staticArray(1, 2, 3)
staticArray(1, 2, 3)
So do many other methods in std.algorithm and std.range - the splitting seems to me a bit artificial. You need a lot stronger motivation for this and I really don't want Phobos to end up with one module for every function. |
/cc @wilzbach the only failure is unrelated:
|
86b9565
to
bdac92c
Compare
std/array.d
Outdated
version(all) | ||
{ | ||
|
||
import std.traits : CommonType, Unqual, isStaticArray; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imports go at the top of modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
|
||
// @nogc: https://issues.dlang.org/show_bug.cgi?id=18439 | ||
//nothrow @safe pure: | ||
nothrow: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid using attributes like this, they are a huge pain later on and Andrei has expressed desire for removing all such uses from Phobos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
@@ -3891,3 +3891,231 @@ unittest | |||
assert(appS.data == "hellow"); | |||
assert(appA.data == "hellow"); | |||
} | |||
|
|||
version(all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superfluous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
{ | ||
import std.range; | ||
|
||
assert(isThrown!Error(2.iota.asStatic!1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThrown
doesn't work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not nothrow
std/array.d
Outdated
|
||
enum a = asStatic!(2.iota); | ||
asStatic!(2.iota).assertSame!int([0, 1]); | ||
asStaticCast!(double, 2.iota).assertSame!double([0, 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use undocumented functions in public examples.
Better to just manually enter in both tests from assertSame here for each call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
assert(isThrown!Error(2.iota.asStatic!3)); | ||
} | ||
|
||
/// ditto but with an alias |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
only works if it's the only thing in the ddoc-ed comment. If you want this to have its own docs, you have to include all the information because the ddox version shows all functions on separate pages, and not in sequential order like ddoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
pure: | ||
|
||
/++ | ||
Returns a static array constructed from `a`. The type of elements can be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs Params:
and Returns:
sections. same throughout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done i think
std/array.d
Outdated
Returns a static array constructed from `a`. The type of elements can be | ||
specified implicitly (`int[2] a = staticArray(1,2);`) or explicitly | ||
(`float[2] a = staticArray!float(1,2)`). | ||
D20180214T203015: The result is an rvalue, therefore uses like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D20180214T203015
What is this? A date? What for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done (was meant as a simple GUID to cross-reference; but removed it)
std/array.d
Outdated
[1, 129].asStaticCast!byte.assertSame!byte([1, -127]); | ||
|
||
version (Bug) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be part of the public documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
} | ||
|
||
// TODO: add this to assertThrown in std.exception | ||
bool isThrown(T : Throwable = Exception, E)(lazy E expression) @system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: There's already https://dlang.org/phobos/std_exception.html#ifThrown
auto foo() {
throw new Exception("test");
return true;
}
ifThrown(foo(), false).writeln;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not nothrow (and results in uglier syntax, see PR in version(none) block)
std/array.d
Outdated
} | ||
|
||
// Note: can't use `version (unittest)` because of https://issues.dlang.org/show_bug.cgi?id=18488 | ||
void assertSame(T, T1, T2)(T1 a, T2 b) @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new public symbol. The tester failed for a good reason. We don't want to use undocumented functionality in public examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed its used from documented unittests
std/array.d
Outdated
/// | ||
@safe unittest | ||
{ | ||
import std.range; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: iota
- for public tests.
Honestly I saw this only after doing my own which does pretty much the same: #6214. Otherwise I probably would have thought something else. |
bdac92c
to
7982c1d
Compare
7982c1d
to
cf9d35e
Compare
f99eaaa
to
c46ab08
Compare
staticArray(1, 2, 3)
staticArray(1, 2, 3) and [1,2].asStatic
c46ab08
to
35581cb
Compare
Here's an idea to avoid being blocked on Andrei's approval for new symbols: we merge this as is to std.experimental and keep it there for one release. We then only need Andrei's approval once every two months when we merge everything from std.experimental to the standard. |
More ideas and a summary: 1)
|
I prefer option 2. If it's good enough for web api (https://developer.mozilla.org/en-US/docs/Web/API) I don't see why it wouldn't be for us. And probably causes the least "drag" in getting stuff in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have pipe.array
to create arrays from pipes, we should probably call this staticArray
.
std/array.d
Outdated
template argument to avoid having to specify the number of elements | ||
(eg: `asStatic!(2.iota)` or `asStatic!(double, 2.iota)`). | ||
|
||
Note: `foo([1, 2, 3].asStatic)` may be inefficient because of the copies involved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessarily about function calls. The matter at hand is asStatic
returns by value, so the text should just remind that. "Note: asStatic
returns by value, so expressions involving large arrays may be inefficient."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
} | ||
|
||
/// ditto | ||
U[n] asStatic(U, T, size_t n)(auto ref T[n] a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge with the previous overload and use static if inside. The user doesn't care that the implementations are different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can replace with
template staticArray(U...) if (U.length == 0 || U.length == 1 && is(U[0]))
{
static if (U.length == 0)
{
pragma(inline, true) T[n] staticArray(T, size_t n)(auto ref T[n] a) nothrow @safe pure @nogc
{
return a;
}
}
else
{
alias U0 = U[0];
U0[n] staticArray(T, size_t n)(auto ref T[n] a)
{
import std.conv : emplaceRef;
U0[n] ret = void;
static foreach (i; 0 .. n)
{
emplaceRef!U0(ret[i], cast(U0) a[i]);
}
return ret;
}
}
}
but I don't see in what ways it improves over what I had:
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)
if (!is(U == T))
{
import std.conv : emplaceRef;
U[n] ret = void;
static foreach (i; 0 .. n)
{
emplaceRef!U(ret[i], cast(U) a[i]);
}
return ret;
}
(more lines of code, more complexity, less readable IMO)
std/array.d
Outdated
if (!is(U == T)) | ||
{ | ||
import std.conv : emplaceRef; | ||
U[n] ret = void; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
}
std/array.d
Outdated
} | ||
|
||
/// ditto | ||
auto asStatic(size_t n, T)(T a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
emplaceRef!U(ret[i++], cast(U) ai); | ||
} | ||
assert(i == n); | ||
return ret; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
|
||
/// ditto | ||
auto asStatic(Un : U[n], U, size_t n, T)(T a) nothrow @safe pure @nogc | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
} | ||
|
||
/// ditto | ||
auto asStatic(alias a)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
std/array.d
Outdated
/// ditto | ||
auto asStatic(alias a)() | ||
{ | ||
return .asStatic!(cast(size_t) a.length)(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is a cast needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise:
Error: template std.array.staticArray cannot deduce function from argument types !(length)(Result)
std/array.d
Outdated
} | ||
|
||
/// ditto | ||
auto asStatic(U, alias a)() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constraint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
--- | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
PTAL, all comments addressed /cc @andralex not sure how one wants to treat cases where ForEachType != ElementType, eg:
|
@timotheecour on strings we should follow the rule of least astonishment - i.e. if you start with a narrow string you get a static string of the same narrowness. Qualifier on the characters should be the same, but no need to make the entire result qualified, i.e. |
Can we please merge this to There are tons of places within Phobos where this would be very useful and we can work out the exact specific and nitpick the name while we get experience using it. I think it goes without saying, but it's a very powerful tool for convenient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple major issues remaining.
|
||
/// ditto | ||
U[n] staticArray(U, T, size_t n)(auto ref T[n] a) nothrow | ||
if (!is(U == T)) |
There was a problem hiding this comment.
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.
} | ||
|
||
/// ditto | ||
U[n] staticArray(U, T, size_t n)(auto ref T[n] a) nothrow |
There was a problem hiding this comment.
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.
// TODO: ElementType vs ForeachType | ||
static foreach (i; 0 .. n) | ||
{ | ||
emplaceRef!U(ret[i], cast(U) a[i]); |
There was a problem hiding this comment.
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.
{ | ||
import std.conv : emplaceRef; | ||
|
||
U[n] ret = void; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFINITELY no cast here
{ | ||
emplaceRef!U(ret[i++], cast(U) ai); | ||
} | ||
assert(i == n); |
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
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
.
@andralex to speed things up, please tell us that once those are resolved, do you want to recheck the code yourself, or is it enough for the other reviewers to accept it? |
@dukc I'd like to take a last look, but the more good reviews the better. |
@timotheecour why the closure? this was a few touches away from merging. |
I may reopen if I find some time later, but right now I don't have time so I don't want to pollute the PR queue |
@timotheecour cool, thanks. Would you allow someone else to take it over? Perhaps @dukc ? |
I was actually considering. I also have more work now than before, but not so much that it's out of question. If this is still waiting the next time I decide to do something, I'll take this. And, of course, if Timothee doesn't want others to do it, I'll resubmit |
@dukc thanks! |
of course |
(moved from dlang/druntime#2093)
usage
motivation
places like here: dlang/dub#1377 (comment)
question for reviewers
not sure how one wants to treat cases where ForEachType != ElementType, eg:
"ab∫".staticArray currently is
immutable(char[5])
instead of immutable(dchar[3])links
https://issues.dlang.org/show_bug.cgi?id=16745 Add template helper for creating static arrays with the size inferred
https://issues.dlang.org/show_bug.cgi?id=8008 Issue 8008 - Syntax for fixed size array literals like [1,2,3]s (edit)
https://issues.dlang.org/show_bug.cgi?id=481 Issue 481 - Fixed-length arrays with automatically computed length (edit)
#4936 Fix Issue 16745 - Add template helper for creating static arrays with the size inferred #4936
#4090 Better static array support in std.array #4090
#6214 Overload for static arrays for std.array.array #6214
not related
https://issues.dlang.org/show_bug.cgi?id=9165 Issue 9165 - Auto conversion from dynamic array to fixed size array at return (edit)
This change is