Static array UFCS-style #6489

dukc · 2018-05-01T17:16:06Z

This is #6178 rebooted, so most of this is work of @timotheecour , not mine. It tries to address the last review @andralex made of it, and also some feedback made to #6214, which is my pull request aiming for same functionality.

dlang-bot · 2018-05-01T17:16:08Z

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

Auto-close	Bugzilla	Severity	Description
✓	16745	enhancement	Add template helper for creating static arrays with the size inferred

Testing this PR locally

If 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#6489"

dukc · 2018-05-02T05:43:16Z

Circleci crashed. Trying to restart...

dukc · 2018-05-02T05:56:42Z

Anybody know how I can fix the Circleci seqfault?

wilzbach · 2018-05-02T06:07:18Z

Unfortunately that's a known issue: https://issues.dlang.org/show_bug.cgi?id=18667

(I just restarted the CI as it only happens in ~ every 20-30 run)

wilzbach · 2018-05-02T06:08:40Z

Oh and you can only restart CircleCi by either

being a member of the D GitHub organization and then logging in directly at their page
git pushes - an empty forced pushed with git commit --amend helps

wilzbach

Not sure about the rangeLength out parameter. Is there a good use case for it?

wilzbach · 2018-05-02T12:50:30Z

std/array.d

+}
+
+/// ditto
+auto staticArray(size_t n, T)(T a, out size_t rangeLength)


Do we need rangeLength? (i.e. is there any good use case?)

Andrei wanted that feature in #6214 review, and because it's so similar I assume the same wish applies here too.

wilzbach · 2018-05-02T12:50:51Z

std/array.d

+        return theArray;
+    }());
+
+    for (auto iter = a.take(n); !iter.empty; iter.popFront())


Why not foreach?

wilzbach · 2018-05-02T12:51:09Z

std/array.d

+    {
+        // Compile-time version to avoid unchecked memory access.
+        Unqual!U[n] ret;
+        for (auto iter = a.take(n); !iter.empty; iter.popFront())


Why not foreach?

It was a ref foreach in #6178, but Andrei pointed out that it woud not work with rvalue elements, and suggested this. Would foreach without ref make a needless copy with lvalue elements? Not sure.

wilzbach · 2018-05-02T12:56:14Z

std/array.d

+        return (() @trusted => cast(U[n]) ret)();
+    }
+
+    auto ret = (() @trusted


Does this need to be @trusted?

I think removing @trusted would prevent the function from being safe if the element type is some pointer. Because they are initialized later, the function should be safe with pointers too.

Ah right. Forgot that using void makes every unsafe. Sorry.

wilzbach · 2018-05-02T12:57:14Z

std/array.d

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


Nit: we typically don't have an empty line here.

dukc · 2018-05-08T12:17:57Z

/cc @andralex still wanted a final review on the issue.

andralex

Getting there. Exceptions safety still not present.

andralex · 2018-05-11T19:12:06Z

std/array.d

+}
+
+pragma(inline, true) U[n] staticArray(U, T, size_t n)(auto ref T[n] a) nothrow @safe pure @nogc
+if (!is(T == U) && is(T : U))


Do we need the first test? is(T : U) should be sufficient.

Aye. Tried without, there was ambiquity with the first overload in this pr.

Oh, indeed. I missed the negation on the first is...

andralex · 2018-05-11T19:16:16Z

std/array.d

+
+    for (auto iter = a.take(n); !iter.empty; iter.popFront())
+    {
+        emplaceRef!U(ret[i++], iter.front);


If an exception is thrown, we're toast - uninitialized values will be incorrectly destroyed. See @timotheecour 's PR where I posted code that can be used to fix that.

dukc · 2018-05-15T08:35:37Z

@andralex exceptions and destructor thing addressed.

Good catch btw! I think no-one else noticed.

andralex

one more look pls

andralex · 2018-05-15T17:34:31Z

std/array.d

+}
+
+pragma(inline, true) U[n] staticArray(U, T, size_t n)(auto ref T[n] a) nothrow @safe pure @nogc
+if (!is(T == U) && is(T : U))


Oh, indeed. I missed the negation on the first is...

andralex · 2018-05-15T17:35:13Z

std/array.d

+
+/// ditto
+auto staticArray(Un : U[n], U, size_t n, T)(T a) nothrow
+if ((isInputRange!T) && is(ElementType!T : U))


redundant parens

andralex · 2018-05-15T17:35:36Z

std/array.d

+}
+
+/// ditto
+auto staticArray(Un : U[n], U, size_t n, T)(T a) nothrow


wait can't this throw if copying objects throws?

Indeed. That's a leftover from Timothee. I think I also need to add a test to prevent errors like this.

andralex · 2018-05-15T17:35:48Z

std/array.d

+
+/// ditto
+auto staticArray(Un : U[n], U, size_t n, T)(T a, out size_t rangeLength)
+if ((isInputRange!T) && is(ElementType!T : U))


redundant parens

dukc · 2018-05-16T07:12:55Z

@andralex "Looked" once more.

dukc · 2018-05-16T07:20:51Z

Btw the previous version did not compile with elaborate destructors, the default-initializator was incorrectly written. But tests didn't catch that because of the static if.

I solved that by putting the same code that default-initializes the rest of the array when the range is too short in scope (exit), so that it should now handle the exceptions as well.

dukc · 2018-05-16T07:29:56Z

@wilzbach CircleCI fails to compile but my local compiler and the autotester do not. What is happening?

wilzbach · 2018-05-16T08:22:17Z

The auto-tester doesn't test with -dip1000, only CircleCi does. You can check locally with the std/array.test target

dukc · 2018-05-16T10:22:13Z

Understood. I have to look at that later.

dnadlinger · 2018-05-18T23:08:46Z

std/array.d

+        return theArray;
+    }());
+
+    if (true)


if (true)?

It's for the scope(exit) inside it. I could use just a regular block statement but if (true) looks better to the eye IMO. Does the style guide say anything about this?

dnadlinger · 2018-05-18T23:10:25Z

std/array.d

+    import std.range : take;
+    import std.conv : emplaceRef;
+
+    size_t i;


I'd declare these is locally next to the loops to make it clearer what the scope is. Stylistic choice, though.

dnadlinger · 2018-05-18T23:12:25Z

changelog/std-array-asStatic.md

+template argument to avoid having to specify the number of elements
+(eg: `staticArray!(2.iota)` or `staticArray!(double, 2.iota)`).
+
+Note: $(D fun([1, 2, 3].staticArray)) may be inefficient because of the copies involved.


Shouldn't we be able to make sure that RVO happens?

Ask Walter, not me. Because the comment was here already I thought there may be a reason for it and decided to leave it there..

I would remove the note from the changelog as I think it's clear that in the general case static arrays need to be copied and such details belong into the detailed documentation of the function.

See also: https://godbolt.org/g/GEsh3E

Oh, i didn't notice that static array was being fed to a function. I thought he meant that [1, 2, 3] might be unnecessarily copied when initializing. Sounded unlikely, but I thought that Timothee might know something I do not.

But since it means that feeding a static array to a function involves a copy, I will remove that.

But looking at that godbolt example, why does LDC make bar to copy the static array when passing it to foo when it does not use it for anything else? Can't it rely on foo to copy it if it mutates it?

dnadlinger · 2018-05-18T23:15:12Z

std/array.d

+@system nothrow unittest
+{
+    // exists only to allow doing something in the destructor
+    static int preventersDestroyed = 0;


Shouldn't this be compared against the expected value later?

I originally did, but it was larger than I excepted. That tells me that the value is implementatation-dependant and thus better to not be tested.

However I decided to add assert (false) after the static array initializer to ensure the copy preventing postblit works, and also added the above rationale to a comment.

dnadlinger · 2018-05-18T23:15:35Z

std/array.d

+
+nothrow pure @safe unittest
+{
+


Superfluous whitespace.

dnadlinger · 2018-05-18T23:16:58Z

std/array.d

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


Can this be size_t(a.length) to avoid accidentally introducing bugs?

dukc · 2018-05-19T05:45:07Z

Thanks for the comments. All explained and/or fixed.

dukc · 2018-05-22T07:12:27Z

@andralex Tests pass and it's ready for another try at review.

dukc · 2018-05-23T12:50:54Z

ping @andralex

wilzbach · 2018-06-08T10:24:52Z

changelog/std-array-asStatic.md

+template argument to avoid having to specify the number of elements
+(eg: `staticArray!(2.iota)` or `staticArray!(double, 2.iota)`).
+
+Note: $(D fun([1, 2, 3].staticArray)) may be inefficient because of the copies involved.


I would remove the note from the changelog as I think it's clear that in the general case static arrays need to be copied and such details belong into the detailed documentation of the function.

See also: https://godbolt.org/g/GEsh3E

wilzbach · 2018-06-08T10:26:12Z

std/array.d

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


unnecessary empty line.

wilzbach · 2018-06-08T10:37:17Z

std/array.d

+}
+
+/// ditto
+auto staticArray(size_t n, T)(scope T a, out size_t rangeLength)


I'm still not sure whether it's a good idea to expose the rangeLength API if there's no good use case for it. Or did you manage to find one?

I believe it was wanted for cases like reading input to a fixed-size buffer. So that one can get the length of the input actually in the buffer without using tee or enumerate.

See: #6178 (comment) and #6214 (review)

dukc · 2018-06-09T07:28:31Z

Okay, addressed.

dukc · 2018-06-15T18:32:35Z

ping @andralex

review has been addressed.

wilzbach

A few final nits.
@dukc would you mind squashing this to one commit afterwards, s.t. we can finally move on?
Unfortunately we missed 2.081, but I really want to see this in 2.082

wilzbach · 2018-06-19T00:21:14Z

changelog/std-array-asStatic.md

+Added `staticArray` to construct a static array from input array / range / CT range.
+
+The type of elements can be specified implicitly ($(D [1, 2].staticArray) of type `int[2]`)
+or explicitly ($(D [1, 2].staticArray!float) of type `float[2]`).


"of" sounds a bit confusing, maybe "will result in an array of type", "results type", "creates type" or "yields"?

wilzbach · 2018-06-19T00:21:35Z

changelog/std-array-asStatic.md

+
+The type of elements can be specified implicitly ($(D [1, 2].staticArray) of type `int[2]`)
+or explicitly ($(D [1, 2].staticArray!float) of type `float[2]`).
+When `a` is a range with length not known at compile time, the number of elements must be given as template argument


s/with/whose/

wilzbach · 2018-06-19T00:21:47Z

changelog/std-array-asStatic.md

+The type of elements can be specified implicitly ($(D [1, 2].staticArray) of type `int[2]`)
+or explicitly ($(D [1, 2].staticArray!float) of type `float[2]`).
+When `a` is a range with length not known at compile time, the number of elements must be given as template argument
+(eg `myrange.staticArray!2`).


wilzbach · 2018-06-19T00:22:19Z

changelog/std-array-asStatic.md

+or explicitly ($(D [1, 2].staticArray!float) of type `float[2]`).
+When `a` is a range with length not known at compile time, the number of elements must be given as template argument
+(eg `myrange.staticArray!2`).
+Size and type can be combined, if source range elements are implicitly


if the source range elements?

wilzbach · 2018-06-19T00:22:37Z

changelog/std-array-asStatic.md

+convertible to the requested element type (eg: `2.iota.staticArray!(long[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)`).


wilzbach · 2018-06-19T00:22:53Z

std/array.d

+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 $(D staticArray!(double, 2.iota))).


wilzbach · 2018-06-19T00:23:07Z

std/array.d

+/++
+Constructs a static array from `a`.
+The type of elements can be specified implicitly ($(D [1,2].staticArray) of type `int[2]`)
+or explicitly ($(D [1,2].staticArray!float) of type `float[2]`).


Same comment about "of"

wilzbach · 2018-06-19T00:23:14Z

std/array.d

+Constructs a static array from `a`.
+The type of elements can be specified implicitly ($(D [1,2].staticArray) of type `int[2]`)
+or explicitly ($(D [1,2].staticArray!float) of type `float[2]`).
+When `a` is a range with length not known at compile time, the number of elements must be given as a template argument


s/with/whose/

wilzbach · 2018-06-19T00:24:09Z

std/array.d

+template argument to avoid having to specify the number of elements
+(eg: `staticArray!(2.iota)` or $(D staticArray!(double, 2.iota))).
+
+Note: `staticArray` returns by value, so expressions involving large arrays may be inefficient.


We already agreed on removing this from the changelog. I think the same argumentation applies for the docs too?

dukc · 2018-06-21T18:16:43Z

@wilzbach Docs corrected. But I don't want to squash, because I don't know how to squash commits by others. I don't want to accidently remove all credit to Timothee!

I let you to squash them, if you want. But please do not remove Timothee.

…th the size inferred

wilzbach · 2018-06-29T20:19:07Z

But I don't want to squash, because I don't know how to squash commits by others. I don't want to accidently remove all credit to Timothee!
I let you to squash them, if you want. But please do not remove Timothee.

Okay squashed them down to two commits (i.e. all your commits into one and commits of Timothee into one commit).

wilzbach · 2018-06-30T18:51:05Z

Auto-merge toggled on

dukc requested review from JackStouffer, wilzbach and PetarKirov as code owners May 1, 2018 17:16

dlang-bot added the Severity:Enhancement label May 1, 2018

dukc force-pushed the staticArray branch 3 times, most recently from 35d43c7 to 8ac17a6 Compare May 1, 2018 17:45

dukc closed this May 2, 2018

dukc reopened this May 2, 2018

dukc force-pushed the staticArray branch 4 times, most recently from 91bfbaf to 8ecb0d2 Compare May 2, 2018 06:58

wilzbach reviewed May 2, 2018

View reviewed changes

dukc force-pushed the staticArray branch from 8ecb0d2 to b79aa57 Compare May 2, 2018 13:22

andralex requested changes May 11, 2018

View reviewed changes

andralex previously requested changes May 15, 2018

View reviewed changes

dukc force-pushed the staticArray branch from 30c75f5 to b0586c3 Compare May 18, 2018 18:54

dukc force-pushed the staticArray branch from 7ba6610 to 359b6c9 Compare May 18, 2018 19:59

dnadlinger reviewed May 18, 2018

View reviewed changes

std/array.d Outdated

nothrow pure @safe unittest

{

Copy link

Contributor

dnadlinger May 18, 2018

Choose a reason for hiding this comment

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

Superfluous whitespace.

dnadlinger reviewed May 18, 2018

View reviewed changes

dukc force-pushed the staticArray branch 2 times, most recently from 2d7125f to 0eedb3c Compare May 19, 2018 05:44

dukc force-pushed the staticArray branch 3 times, most recently from ed55a66 to 0fd213f Compare May 19, 2018 10:55

wilzbach reviewed Jun 8, 2018

View reviewed changes

dukc force-pushed the staticArray branch from 0fd213f to 827c36a Compare June 9, 2018 07:23

wilzbach reviewed Jun 19, 2018

View reviewed changes

dukc force-pushed the staticArray branch from 827c36a to d31bf2f Compare June 21, 2018 18:12

timotheecour and others added 2 commits June 29, 2018 22:18

Fixes Issue 16745 - Add template helper for creating static arrays wi…

e191a7d

…th the size inferred

Addressed Andrei's review.

1688103

wilzbach force-pushed the staticArray branch from d31bf2f to 1688103 Compare June 29, 2018 20:18

wilzbach added the Merge:auto-merge label Jun 29, 2018

wilzbach merged commit 90a8fc3 into dlang:master Jun 30, 2018

		}

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

Uh oh!

Static array UFCS-style #6489

Static array UFCS-style #6489

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dlang-bot commented May 1, 2018

Bugzilla references

Testing this PR locally

Uh oh!

dukc commented May 2, 2018

Uh oh!

dukc commented May 2, 2018

Uh oh!

wilzbach commented May 2, 2018

Uh oh!

wilzbach commented May 2, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc May 2, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc May 2, 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!

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 May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc May 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dukc commented May 15, 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!

dukc commented May 16, 2018

Uh oh!

dukc commented May 16, 2018

Uh oh!

dukc commented May 1, 2018 •

edited

Loading

dukc May 2, 2018 •

edited

Loading

dukc May 2, 2018 •

edited

Loading

dukc commented May 8, 2018 •

edited

Loading

dukc May 15, 2018 •

edited

Loading

dukc commented May 16, 2018 •

edited

Loading

dukc May 19, 2018 •

edited

Loading

dnadlinger May 18, 2018 •

edited

Loading

dukc May 19, 2018 •

edited

Loading

dukc commented May 22, 2018 •

edited

Loading