Overload for static arrays for std.array.array by dukc · Pull Request #6214 · dlang/phobos

dukc · 2018-02-23T19:32:52Z

With static arrays, auto keyword has been hard to use so far. This changes today!

dlang-bot · 2018-02-23T19:32:54Z

Thanks for your pull request and interest in making D better, @dukc! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

dukc · 2018-02-23T19:33:35Z

@andralex opinion needed because this is a new symbol overload.

andralex

We should be able to transport the information about how many elements were filled, should client code need it. I'm thinking of an overload with out size_t as the last parameter.

andralex · 2018-02-23T19:39:54Z

std/array.d

+        size_t i;
+        foreach (e; r)
+        {
+


I copied that comment from regular .array. I think emplaceRef calls memCpy.

andralex · 2018-02-23T19:40:15Z

std/array.d

+/**
+ * Returns a static array from the stack with length of s and initializes
+ * it with copies of the elements of range $(D r). The remainder of the
+ * array is default-initialized.


What happens if the range is longer than s?

A standard overflow. But i think you meant that I add that in comment.

andralex · 2018-02-23T19:41:07Z

std/array.d

+ * Returns a static array from the stack with length of s and initializes
+ * it with copies of the elements of range $(D r). The remainder of the
+ * array is default-initialized.
+ *


The documentation should add that this is recommended particularly for nontrivial initialization of static arrays during compilation.

andralex · 2018-02-23T19:41:51Z

std/array.d

 }

+/**
+ * Returns a static array from the stack with length of s and initializes


s in backquotes

andralex · 2018-02-23T19:42:31Z

std/array.d

+ *      An initialized static array
+ */
+ForeachType!Range[s] array(size_t s, Range)(Range r)
+if (isInputRange!Range && !isInfinite!Range)


You don't care whether the range is finite - you take a finite subset anyway.

That's if I change it to check that. Now it goes unchecked so passing an infinite range is definitely wrong.

JackStouffer · 2018-02-23T19:55:24Z

This somewhat overlaps with #6178

dukc · 2018-02-23T20:01:27Z

Yea, I should probably look around before starting doing.

EDIT: even closer is #4090, but it was dismissed by Andrei because it (or this) could be used to initialize a dynamic array and then return it out of function. So this should be dismissed too...

...except that we now have -dip1000 which stops that, if used. but the switch is still not the default... you decide.

…rrays in UFCS style.

JackStouffer · 2018-02-23T21:37:54Z

DIP1000 in Phobos has been "just around the corner" for over a year now. If the same safety issues are present in this implementation, I would close this.

dukc · 2018-02-23T21:44:55Z

Well, we have Andrei around so I'll let him do it if he agrees with that.

dukc · 2018-02-27T11:22:27Z

@andralex Review addressed and the overload you requested has been added. On the other hand, check last three comments above this before merging.

andralex

getting there

andralex · 2018-03-02T16:25:04Z

std/array.d

+ * Params:
+ *      r = The range (or aggregate with $(D opApply) function) whose elements
+ *      are copied into the stack-allocated array. Must be as long or shorter
+ *      than `s`, otherwise causes an array overflow.


Please no overflow. It's trivial and low cost to have a test here. In case of overflow, just fill the array and place the real length in rangeLength.

Take would be easy to add if the user wants it, but not to remove, so this makes it more-than-zero cost abstraction.

On the other hand, we can always add a non-checking equivalent later so I tend to agree with this. Regardless, I'll do it.

andralex · 2018-03-02T16:25:46Z

std/array.d

+    import std.conv : emplaceRef;
+    import std.algorithm.mutation : uninitializedFill;
+
+    rangeLength = 0;


out parameters are always zero-initialized

andralex · 2018-03-02T16:26:41Z

std/array.d

+        Unqual!E[s] result;
+        foreach (e; r)
+        {
+


Please no spurious empty lines. This particular one separates nothing in particular and adds negative readability value.

andralex · 2018-03-02T16:27:35Z

std/array.d

+    }());
+
+
+    foreach (e; r)


This makes a copy for each element in the range.

andralex · 2018-03-02T16:27:58Z

std/array.d

+    foreach (e; r)
+    {
+        emplaceRef!E(result[rangeLength], e);
+        rangeLength++;


Incrementing via pointer is slow, please use a local value and assign rangeLength at end

dukc · 2018-03-05T10:53:06Z

@andralex All done now. Also extended the first unittest a bit.

andralex · 2018-03-06T21:06:28Z

I think we need to choose this, #6178, or a careful combination, but not both. Thoughts?

dukc · 2018-03-07T12:30:10Z

It seems Cour already stated his opinion at #6178 so I decided it's a better place for discussion. See my answer there.

dukc · 2018-03-22T08:56:47Z

Not needed anymore as #6178 appears to be getting in.

dukc requested review from JackStouffer, PetarKirov and wilzbach as code owners February 23, 2018 19:32

andralex reviewed Feb 23, 2018

View reviewed changes

dukc mentioned this pull request Feb 23, 2018

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

Closed

dukc force-pushed the arrayForStatic branch from 5e0861e to 1b6ce53 Compare February 23, 2018 19:59

Created an overload for std.array.array that enables to make static a…

7babaff

…rrays in UFCS style.

dukc force-pushed the arrayForStatic branch 2 times, most recently from 01b9885 to 6d7ad05 Compare February 23, 2018 21:18

dukc force-pushed the arrayForStatic branch from 6d7ad05 to e90dd8e Compare February 24, 2018 08:55

andralex requested changes Mar 2, 2018

View reviewed changes

Added the overload for forwarding range length as suggested by Andrei

8667c0a

dukc force-pushed the arrayForStatic branch from e90dd8e to 8667c0a Compare March 5, 2018 10:52

dukc closed this Mar 22, 2018

dukc mentioned this pull request May 1, 2018

Static array UFCS-style #6489

Merged

Uh oh!

Comments

Conversation

dukc commented Feb 23, 2018

Uh oh!

dlang-bot commented Feb 23, 2018

Bugzilla references

Uh oh!

dukc commented Feb 23, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackStouffer commented Feb 23, 2018

Uh oh!

dukc commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Feb 23, 2018

Uh oh!

dukc commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dukc commented Feb 27, 2018

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented Mar 6, 2018

Uh oh!

dukc commented Mar 7, 2018

Uh oh!

dukc commented Mar 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dukc Feb 23, 2018 •

edited

Loading

dukc commented Feb 23, 2018 •

edited

Loading

dukc commented Feb 23, 2018 •

edited

Loading

dukc commented Mar 5, 2018 •

edited

Loading