Fix constness of array.d #2631

nordlow · 2014-10-20T19:36:05Z

Thanks to @aG0aep6G this works as far as I can see.

nordlow · 2014-10-20T20:00:17Z

For reference see http://forum.dlang.org/thread/vbmwhzvawhnkoxrhbnyb@forum.dlang.org?page=1

DmitryOlshansky · 2014-10-24T04:39:09Z

std/container/array.d

        }
-
-        @property ref inout(T) back() inout
+        else


What if elements are immutable? Then things like arr[].filter(x > 0).array degrade to const element.

(Not related to DmitryOlshansky's comment.)

Looks like I've missed an else here. There should be no elses to the static if (isMutable!A)s.

Should we add another

else static if (is(T == immutable)) { @property ref immutable(T) front() const { version (assert) if (empty) throw new RangeError(); return _outer[_a]; } @property ref immutable(T) back() const { version (assert) if (empty) throw new RangeError(); return _outer[_b - 1]; } }

then?

JakobOvrum · 2014-10-25T15:03:01Z

Array.Range is a public type that is used as a parameter type of many of Array's methods, so this is a pretty serious breaking change.

JakobOvrum · 2014-10-25T15:07:47Z

I don't think adding all this duplicated code is a nice solution to this problem. The fundamental issue is that we have conversions for const(T[]) to const(T)[] but no equivalent way to implement const(Array!T) to Array!(const T) (and ditto for immutable).

aG0aep6G · 2014-10-25T16:09:39Z

JakobOvrum wrote:

Array.Range is a public type that is used as a parameter type of many of Array's methods, so this is a pretty serious breaking change.

We could get around the breakage by renaming the range template to RangeT or something, and adding an alias Range = Range!T(Array);.

nordlow · 2014-10-26T21:28:55Z

@aG0aep6G I guess you mean

alias Range(T) = RangeT!(Array!T);

right?

aG0aep6G · 2014-10-26T22:05:51Z

(...)

I meant alias Range = RangeT!(Array);. Alternatively, alias Range = RangeT!(Array!T) would be equivalent. But no (T) on the left side.

And it has to go into Array, because that's where it's been the whole time.

The point is that Array!Foo.Range has been a concrete type so far, and should stay that way.

aG0aep6G · 2014-10-27T21:18:16Z

I made a pull request against your branch with what I mean: nordlow#2

nordlow · 2014-10-27T23:19:57Z

Sigh, I got back the same compilation errors after merging #2643. I don't understand what I did wrong. How do I ignore edits from pull 2643 when I merge or rebase?

aG0aep6G · 2014-10-28T14:37:22Z

What errors? What do you want to do? What did you do?

nordlow · 2014-10-28T20:42:43Z

My rebase gave merge conflicts beyond what I could resolve because someone did a revert of previous work on the constness of array.d at #2643. I start over and see what you guys think this time.

aG0aep6G · 2014-10-28T21:19:34Z

When throwing away everything from #2643, you also deleted this test: https://github.com/Dicebot/phobos/commit/1149972daca5290ed401dcd0a03f3a7eb5c5074c. Better put it back in.

nordlow · 2014-10-28T21:49:58Z

Done. BTW: Don't we need a specific test for immutable Arrays and/or Ranges?

aG0aep6G · 2014-10-30T22:25:55Z

I have a suggestion that's rather invasive. So I made a pull request: nordlow#3

The commit message:

T misses A's qualifiers which are needed. E has them. This means,
const/immutable are taken over from A automatically. This allows inout
to be used on front/back/opIndex. inout doesn't work for opSlice, but the
isMutable!A guard can be dropped.

move*, opSliceAssign, etc would be strictly fine with T, but to avoid
confusion I think it's better to not use T in Range at all.

This gets us rid of quite some duplication. Also adds tests for const/immutable Arrays/Ranges.

MartinNowak · 2014-10-31T12:54:50Z

I don't think adding all this duplicated code is a nice solution to this problem. The fundamental issue is that we have conversions for const(T[]) to const(T)[] but no equivalent way to implement const(Array!T) to Array!(const T) (and ditto for immutable).

True, but there doesn't seem to be an alternative solution.
What you actually want here is covariance w.r.t. template arguments, e.g. Array!T implicitly converts to Array!(const T).
In Scala you'd write Array[+T] to express that.

MartinNowak · 2014-10-31T14:04:35Z

I'd really like to have a solid solution to this problem as it's a recurring problem and resorting to Range, ConstRange and ImmutableRange (const_iterator) is pathetic.
This is the best idiom I could come up with, but it segfaults the compiler (Issue 13667).
http://dpaste.dzfl.pl/70f17b04ee6d
Maybe it's worth to post this on the newsgroup as someone might have a better idea or has already solved the problem.

nordlow · 2014-11-02T21:26:32Z

Is this or nordlow#3 the way to go forward with this issue? I don't understand why Github says that nordlow#3 is pending. Hasn't this already been merged by me?

aG0aep6G · 2014-11-03T00:24:53Z

Everything's here. I'm closing nordlow#3. Maybe you didn't pull it through Github or something.

nordlow · 2014-11-03T20:45:49Z

Great. Ready to merge?

JakobOvrum · 2014-11-26T19:01:35Z

std/container/array.d

        }

-        void opSliceAssign(T value)
+        RangeT!(const(A)) opSlice() const


Is there a reason why these are const instead of using inout?

Changing to inout triggers a compiler bug and errors as

array.d(235,22): Error: variable std.container.array.Array!int.Array.RangeT!(inout(Array!int)).RangeT._outer_ only parameters or stack based variables can be inout array.d(235,22): Error: variable std.container.array.Array!int.Array.RangeT!(inout(const(Array!int))).RangeT._outer_ only parameters or stack based variables can be inout array.d(333,9): Error: template instance std.container.array.Array!int.Array.RangeT!(inout(const(Array!int))) error instantiating array.d(328,9): instantiated from here: RangeT!(inout(Array!int)) array.d(386,19): instantiated from here: RangeT!(Array!int) array.d(898,5): instantiated from here: Array!int array.d(333,9): Error: template instance std.container.array.Array!int.Array.RangeT!(const(Array!int)) error instantiating array.d(386,19): instantiated from here: RangeT!(Array!int) array.d(898,5): instantiated from here: Array!int array.d(927,5): Error: static assert (!true) is false

JakobOvrum · 2014-11-26T19:06:21Z

All the positional insert and remove methods now only work when given mutable positions (i.e. Array.Range), which may or may not make sense.

DmitryOlshansky · 2014-11-27T03:07:02Z

std/container/array.d

-Defines the container's primary range, which is a random-access range.
-     */
-    static struct Range
+    private static struct RangeT(A)


If doesn't depend on T move it out of Array strict.
No need to spread the bloat by putting a template in a template.

I believe there are different views about this. Do @MartinNowak @Dicebot @yebblies @JakobOvrum agree on this? I've already made this refactoring but some reviewer wanted me to revert it. I don't remember who.

Yes, I with @DmitryOlshansky here - unnecessary nesting of templates creates potential combinatoric template bloat and no D compiler is currently capable of merging identical binary blocks.

Yeah, it's also a little confusing from a readability perspective.

I really wish this type didn't have to be a template to solve this problem, but I don't know of any solutions myself...

DmitryOlshansky · 2014-11-27T03:11:33Z

Looking good overall

nordlow · 2014-11-30T21:39:27Z

And rebased.

nordlow · 2014-12-02T00:26:19Z

Moved RangeT to global scope as requested.

aG0aep6G · 2014-12-07T12:12:04Z

On Wed, Nov 26, 2014 at 8:06 PM, JakobOvrum notifications@github.com
wrote:

All the positional insert and remove methods now only work when given
mutable positions (i.e. Array.Range), which may or may not make sense.

This hasn't changed, has it? They took a (mutable) Range before, too. Maybe
we could lift that restriction, but I think that should be done in another
pull.

Like before, one has to obtain a Range from a mutable Array for mutating
operations. This may not be strictly necessary, but it's fairly reasonable.

nordlow · 2014-12-08T09:15:11Z

This hasn't changed, has it? They took a (mutable) Range before, too. Maybe

I'm not sure what is the best choice here regarding mutablity of Range.

we could lift that restriction, but I think that should be done in another
pull.

I vote for a separate pull.

aG0aep6G · 2014-12-28T23:21:19Z

Ping again?

nordlow · 2015-01-15T10:09:04Z

Is there something you expect me to add here?

If the problem is mutability issues could you please show me a unittest that currently fails that should pass.

I'm not skilled enough in Phobos policies to decide whether positional inserts and removes should require mutability of Range or not. @andralex @WalterBright ?

nordlow · 2015-01-20T16:37:08Z

Ping.

quickfur · 2015-01-20T17:01:53Z

ping @DmitryOlshansky @JakobOvrum @MartinNowak @andralex

JakobOvrum · 2015-01-28T15:50:20Z

std/container/array.d


 public import std.container.util;

+private static struct RangeT(A)


static for structs is a no-op at module level

Shall I remove?

Yes please :)

JakobOvrum · 2015-01-28T15:52:40Z

I'd still prefer a more general solution involving conversions between const(C.Range!T) and C.Range!(const T), but this patch seems forward-compatible with such a solution (except possible deprecation of C.ConstRange and C.ImmutableRange), so LGTM.

JakobOvrum · 2015-01-28T15:54:26Z

I'll merge it in a day or so unless someone finds new evidence against it until then.

JakobOvrum · 2015-01-28T19:14:28Z

Quite a few commits here, please squash.

andralex · 2015-01-28T20:51:35Z

squash

andralex · 2015-01-28T20:51:52Z

after that green light @JakobOvrum

nordlow · 2015-01-28T22:15:07Z

Done. I did an interactive rebase. I hope that is what you wanted me to do.

JakobOvrum · 2015-01-29T06:38:52Z

Auto-merge toggled on

Fix constness of array.d

DmitryOlshansky reviewed Oct 24, 2014
View reviewed changes

nordlow force-pushed the inout-array-range branch from ad0fce9 to 6fd1646 Compare October 26, 2014 20:48

nordlow mentioned this pull request Oct 27, 2014

Use inout for ref returning members in std.container.array #2573

Merged

nordlow force-pushed the inout-array-range branch from e65acca to 81eb394 Compare October 28, 2014 21:01

nordlow force-pushed the inout-array-range branch from 81eb394 to 8a14120 Compare October 28, 2014 21:49

nordlow force-pushed the inout-array-range branch from 8a14120 to 4bb3cd7 Compare November 1, 2014 09:04

JakobOvrum reviewed Nov 26, 2014
View reviewed changes

DmitryOlshansky reviewed Nov 27, 2014
View reviewed changes

nordlow force-pushed the inout-array-range branch from 9bcd503 to 0fdf081 Compare November 30, 2014 21:39

nordlow force-pushed the inout-array-range branch from 0fdf081 to 744c704 Compare December 2, 2014 00:25

JakobOvrum reviewed Jan 28, 2015
View reviewed changes

Fix constness of std.container.Array

14d928e

nordlow force-pushed the inout-array-range branch from f942e81 to 14d928e Compare January 28, 2015 22:14

JakobOvrum added a commit that referenced this pull request Jan 29, 2015

Merge pull request #2631 from nordlow/inout-array-range

224238a

Fix constness of array.d

JakobOvrum merged commit 224238a into dlang:master Jan 29, 2015

nordlow deleted the inout-array-range branch January 29, 2015 20:19

aG0aep6G mentioned this pull request Jul 21, 2015

Const opSlice, upperBound, lowerBound and equalRange for rbtree #3501

Merged


		public import std.container.util;

		private static struct RangeT(A)

Uh oh!

Fix constness of array.d #2631

Fix constness of array.d #2631

Uh oh!

Conversation

nordlow commented Oct 20, 2014

Uh oh!

nordlow commented Oct 20, 2014

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!

JakobOvrum commented Oct 25, 2014

Uh oh!

JakobOvrum commented Oct 25, 2014

Uh oh!

aG0aep6G commented Oct 25, 2014

Uh oh!

nordlow commented Oct 26, 2014

Uh oh!

aG0aep6G commented Oct 26, 2014

Uh oh!

aG0aep6G commented Oct 27, 2014

Uh oh!

nordlow commented Oct 27, 2014

Uh oh!

aG0aep6G commented Oct 28, 2014

Uh oh!

nordlow commented Oct 28, 2014

Uh oh!

aG0aep6G commented Oct 28, 2014

Uh oh!

nordlow commented Oct 28, 2014

Uh oh!

aG0aep6G commented Oct 30, 2014

Uh oh!

MartinNowak commented Oct 31, 2014

Uh oh!

MartinNowak commented Oct 31, 2014

Uh oh!

nordlow commented Nov 2, 2014

Uh oh!

aG0aep6G commented Nov 3, 2014

Uh oh!

nordlow commented Nov 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Nov 26, 2014

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!

DmitryOlshansky commented Nov 27, 2014

Uh oh!

nordlow commented Nov 30, 2014

Uh oh!

nordlow commented Dec 2, 2014

Uh oh!

aG0aep6G commented Dec 7, 2014

Uh oh!

nordlow commented Dec 8, 2014

Uh oh!

aG0aep6G commented Dec 28, 2014

Uh oh!

nordlow commented Jan 15, 2015

Uh oh!

nordlow commented Jan 20, 2015

Uh oh!

quickfur commented Jan 20, 2015