-
-
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
Better static array support in std.array #4090
Conversation
* | ||
* Returns: static array of size `T.length` | ||
*/ | ||
auto staticArray(T...)(T values) |
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.
CommonType!T[T.length] staticArray(T...)(T values) if (is(CommonType!T))
{
typeof(return) result = [values];
return result;
}
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.
Doh. Yeah, that's much simpler. Thanks!
On March 16, 2016 11:08:51 PM EDT, JakobOvrum notifications@github.com wrote:
- }
- int[5] a = OpApply().array!5;
- assert(a == [ 0, 1, 2, 3, 4 ]);
+}
+/**
- * Initialize a static array from the given elements (essentially a
static- * array literal).
- * Params:
- * values = elements of the static array
- * Returns: static array of size
T.length
- */
+auto staticArray(T...)(T values)CommonType!T[T.length] staticArray(T...)(T values) if (is(CommonType!T)) { typeof(return) result = [values]; return result; }
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
https://github.com/D-Programming-Language/phobos/pull/4090/files/67c6a7b1ae5a0f41c43c45f80048a19be469e77f#r56450014
Sent from my Android device with K-9 Mail. Please excuse my brevity.
This is pretty sweet - I like it too. LGTM :) |
As your example shows, auto a = staticArrayFromRange!(only(1,2,3,4,5));
static assert(is(typeof(a) == int[5])); |
@schuetzm I'm trying to adapt this to handle a range, but I'm not sure its possible.
The compiler complains that There was talk about a template to convert any range to an |
Scratch that, it does work in the general case. I just need to handle immutable collections (my test was failing on a |
@schuetzm see if the latest commit is what you're looking for. I just decided to replace I think I'll need to do something like |
Try |
if (isIterable!(typeof(range)) && hasLength!(typeof(range))) | ||
{ | ||
alias T = ForeachType!(typeof(range)); | ||
enum N = range.length; |
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 length
needs to shrink as the range is iterated and this function requires length
to be available at CT, it means that other fields of range
are available at CT (it's not, for example, an alias to a local variable). Then you can build the fixed-length array like so:
auto staticArray(alias range)()
{
enum len = range.length;
ElementType!(typeof(range))[len] result = [aliasSeqOf!range];
return result;
}
Explicit return type sadly doesn't work due to a bug - the compiler fails to recognize range.length
as compile-time readable when put T[here]
yet works as a manifest constant initializer. If it did work, then it could also easily be implemented more faithfully to what it actually does, as a manifest constant template:
enum ElementType!(typeof(range))[range.length] staticArray(alias range) = [aliasSeqOf!range];
That said, since we have aliasSeqOf
I think the previous interface using a variadic argument list is better, for a couple of reasons, but primarily because an alias sequence can be any mix of compile-time and runtime elements, while this interface requires all elements to be CT-readable.
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.
With a variadic argument list, if we pass a single range, we cannot tell whether we want an array with the range itself as only element, or an array with the range's elements. As far as I can see, there need to be two differently named functions for that.
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 PR does exactly that though, with two differently named functions.
I fail to see a use for IMO, overloading |
@JackStouffer, it's been discussed before, it enables creation of fixed-length arrays without having to explicitly specify the element type and length. A language proposal to enable the same, as edit: An additional advantage is that |
Ok, but I still think |
BTW, what happened to this proposal:
If I'm not mistaken even Andrei thought that it is a better idea than adding the |
I'm fond of I'm ok with calling it something more verbose if people find that preferable. @ZombineDev I'm not sure how that would work. The static array length needs to |
@JakobOvrum I reverted back to the variadic |
@rcorre It's actually quite simple: http://dpaste.dzfl.pl/48720ac4df8f |
@ZombineDev whoah, magic! Didn't realize there was an implicit conversion from an array literal to a static array argument (which makes sense, considering how static array declarations work). How do people feel about that? I prefer it to the variadic approach. Is the name |
|
@JackStouffer how's this?
|
Looks great! |
@@ -342,6 +345,105 @@ then the result will contain the value of the last pair for that key in r. | |||
See_Also: $(XREF typecons, Tuple) | |||
*/ | |||
|
|||
/** | |||
* Initialize a static array of the given size from the range $(D r). |
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.
Nitpick: initializes
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.
Actually: "Returns a static ... initialised from ..."
There is also another interesting way of expressing this in D which mimics the built-in syntax, which is kind of neat: |
@MetaLang clever! I don't think its worth the added verbosity but it is cool. |
Besides |
@ntrel good point about initialization. Supporting immutable elements may be more difficult... |
* If `r` has less than `n` elements, only the first `n` are copied, and the | ||
* remaining elements are default-initialized. | ||
* | ||
* A range violation will occur if `r` has more than `n` elements. |
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 update, perhaps replace these 2 sentences with:
Preconditions:
`r` must have at least `n` elements.
LGTM apart from the mentioned doc update, and personally I would revert 7a7e163 as it makes the examples harder to read. The user will understand that One thing: I would put |
Can't we make this as an extra flag or argument?
You might also use StoppingPolicy. |
|
||
// extend the range to the desired length before handing it to staticArray | ||
auto arr = only(1,2,3).chain(0.repeat).staticArray!5; | ||
assert(arr == [ 1, 2, 3, 0, 0 ]); |
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.
what happens if there are different elements in only
?
Worth a test?
The second overload could be useful if this pattern was used enough, but I'm not convinced. I suggest we stick with the current chain/repeat example in this pull:
chain without repeat is also more flexible. If later this turns out to be awkward, we can add the 2nd overload. |
I agree. I actually considered a flag, but I'd rather keep the signature as simple as possible. I'll take a look at your doc suggestions later, they sound good. |
Convinced ;-) |
This reverts commit 7a7e163.
Place the simpler overload first.
@ntrel I took all your suggestions. Thanks! |
*/ | ||
@nogc T[n] staticArray(T, size_t n)(T[n] arr) | ||
{ | ||
pragma(inline, true); |
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 should be right before the function declaration.
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.
Ok, you're the second person to say that so I guess I'll switch it.
But the docs and all but two existing uses in phobos use pragma(inline, true);
inside the function body. Try ack 'pragma\(inline'
. Not to say that means its right.
|
@ZombineDev this actually came up on the forums. |
This is an unsuitable abstraction - it comes with as many invalid as valid uses. I think we should add this: @nogc ref auto staticArrayRef(values...)()
{
import std.traits;
static CommonType!values[values.length] result = [ values ];
return result;
} That offers static storage with auto-computed length, initialized statically. It returns a reference which is nice - it can be used as is or copied. Possible APIs on top of that include run-time arguments etc. But the point is we must be looking for safe APIs that are also convenient, not convenient APIs that have safety issues. I'll close this, feel free to reopen with new code or open another one. |
@andralex is it wise to be storing every static array a user might care to make for the length of the program? It seems to me like this could cause memory usage issues. Also note that this is only unsafe because D foolishly allows implicit conversion of static to dynamic arrays. |
I'm ok with either signature for creating a static array literal. However, is the range-to-static-array overload out of the question? Or would it be acceptable if the DMD bug is resolved? Also, @MetaLang, since Andre's proposed signature only allows creating literals, I think its not any different than allowing users to declare dynamic array literals. |
AFAIK the only invalid case is one that dmd could and should be generating an error for: I suggest reopening this.
BTW This would need to return
|
So does this, does it not?:
My point is that Andrei's signature is purely for creating compile-time literals, similar to how we have builtin support for dynamic array literals (which are also stored statically).
It would only be the static arrays the user defines at compile-time, as this signature disallows creation at runtime (which is a limitation). But I'm kinda playing devil's advocate here; I'd still prefer the runtime syntax if possible, and I'd definitely like the range overload. |
OK, you're right ;-) Updated my comment.
A problem with making staticArrayRef return const is that
|
@andralex staticArrayRef!(1)[0]++;
auto sa = staticArrayRef!(1);
assert(sa[0] == 1); //fails |
Assuming that the DMD bug is the only blocker on this, I may take a stab at it. I know nothing about DMD dev but I've been wanting to get involved. If the DMD bug is fixed, would both of the current overloads be acceptable? If so, would there be any use for |
There is however still a use for it; the returned lvalue it can be passed directly to a function |
Better static array support in
std.array
.The main change I wanted to introduce is
array!n
, which functions likearray
but creates a static array.For example:
This also implements
staticArray
, which is a sort of library-level static array literal:I'm mostly interested in
array!size
, but figured I'd throw instaticArray
as well just to see what people think.