On Monday, 11 November 2019 at 10:27:26 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1025, "Dynamic Arrays Only Shrink, Never Grow":
>
> https://github.com/dlang/DIPs/blob/1b525ec4c914c06bc286c1a6dc93bf1533ee56e4/DIPs/DIP1025.md
>
> All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on November 25, or when I make a post declaring it complete.
>
> At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round of Community Review. Otherwise, it will be queued for the Final Review and Formal Assessment.
>
> Anyone intending to post feedback in this thread is expected to be familiar with the reviewer guidelines:
>
> https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md
>
> *Please stay on topic!*
>
> Thanks in advance to all who participate.

I genuinely believe this is a wrong approach to this. The code breakage is potentially massive on this. It would be better to create a new dynamic array that only shrinks and never grow as a library solution.

Why can't his be done via a library?

-Alex

On 11/11/2019 3:17 AM, Jonathan M Davis wrote:
> Ouch. This would be a _huge_ breaking change. The ability to grow dynamic
> arrays is used all over the place. It's also one of the great benefits of
> D's dynamic array IMHO. The fact that you append to a dynamic array without
> caring where it came from can be incredibly useful, and if you really care
> about checking whether it has the capacity to grow or whether it's managed
> by the GC, we have functions to check for that.

While:

   a ~= b;

would no longer be allowed,

   a = a ~ b;

would be. Yes, it is a breaking change, but the fix for when that behavior is desired is pretty simple.

On 11/11/2019 3:26 AM, Nicholas Wilson wrote:
> The breaking changes are massive and not well argued for.
> The suggested workaround of
> 
> 1) a = a ~ b;
> 
> in place of
> 
> 2) a ~= b;
> 
> is functional identical, thus any issues present in 2 are also present in 1.

The point is making it obvious that the GC is being used, i.e. it is intentional behavior.

On 11/11/19 4:32 PM, Walter Bright wrote:
> On 11/11/2019 3:17 AM, Jonathan M Davis wrote:
>> Ouch. This would be a _huge_ breaking change. The ability to grow dynamic
>> arrays is used all over the place. It's also one of the great benefits of
>> D's dynamic array IMHO. The fact that you append to a dynamic array without
>> caring where it came from can be incredibly useful, and if you really care
>> about checking whether it has the capacity to grow or whether it's managed
>> by the GC, we have functions to check for that.
> 
> While:
> 
>     a ~= b;
> 
> would no longer be allowed,
> 
>     a = a ~ b;
> 
> would be. Yes, it is a breaking change, but the fix for when that behavior is desired is pretty simple.

This is not the same. Appending has amortized cost, while concatenation is not. Expect to have D applications which use this approach for updating explode in runtime.

-Steve

On 11/11/2019 5:13 AM, Dennis wrote:
> On Monday, 11 November 2019 at 11:26:49 UTC, Nicholas Wilson wrote:
>> is functional identical, thus any issues present in 2 are also present in 1. The suggestion that doing so in a loop would create more garbage is false as it is functionally identical.
> 
> There is a difference: a = a ~ b is guaranteed to make a copy while a ~= b might just use spare capacity of the memory that slice `a` refers to.
> 
> https://dlang.org/spec/arrays.html#array-concatenation

That's right. I had overlooked that.


> This is what the dead/alive example is supposed to express.
> It is still true however that the rationale is not very convincing.
> The first example indeed doesn't show any benefits of the DIP:
> 
> ```
> int[] slice = cast(int*)malloc(10 * int.sizeof)[0 .. 10];
> slice = slice ~ 1; // now guaranteed to make a copy
> free(slice.ptr); // Still oops
> ```

Imagine these 3 lines are spread out over a large code base.


> The following claim in the DIP is also unsubstantiated:
> 
>> This change is a necessary part of D evolving towards being memory safe without using
>> a GC.

Memory safety cannot be achieved without control over who is the owner of memory.


> I would like to see an example of memory corruption in `@safe` code that can happen because of slice appending.

  @trusted void myfree(void* p) { free(p); }
  free(slice.ptr);

On Monday, 11 November 2019 at 10:27:26 UTC, Mike Parker wrote:
> This is the feedback thread for the first round of Community Review for DIP 1025, "Dynamic Arrays Only Shrink, Never Grow":
>
> https://github.com/dlang/DIPs/blob/1b525ec4c914c06bc286c1a6dc93bf1533ee56e4/DIPs/DIP1025.md
>
> All review-related feedback on and discussion of the DIP should occur in this thread. The review period will end at 11:59 PM ET on November 25, or when I make a post declaring it complete.
>
> At the end of Round 1, if further review is deemed necessary, the DIP will be scheduled for another round of Community Review. Otherwise, it will be queued for the Final Review and Formal Assessment.
>
> Anyone intending to post feedback in this thread is expected to be familiar with the reviewer guidelines:
>
> https://github.com/dlang/DIPs/blob/master/docs/guidelines-reviewers.md
>
> *Please stay on topic!*
>
> Thanks in advance to all who participate.

As noted by the other reviewers,
- the restrictions proposed by the DIP imply much breaking changes.
- the problem is a lack of distinction between slices and dynamic arrays.

To that I also add that
- the restrictions would be disruptive for a certain usage of D, especially shell-like programs, manipulating array of files, array of directories, program arguments, etc.

so maybe that

1. At the local scope or when this is semantically possible, the compiler could keep track of the difference between a dynamic array and a slice, preventing growing.
2. When not possible anymore, e.g function parameter, a @slice attribute could be used to apply the same restriction as in 1.

---
void func1(int[] arr)
{
    arr ~= 1;               // OK
    arr.length += 1;        // OK
    auto slice = arr[0..2];
    slice.length += 1;      // NG, local restriction (1.)
}

void func2(@slice int[] arr)
{
    arr ~= 1;               // NG, @slice attribute (2.)
    arr.length += 1;        // NG, @slice attribute (2.)
    auto slice = arr[0..2];
    slice.length += 1;      // NG, local restriction (1.)
}
---

As an alternative, instead of 2. the new semantics could be applied only if the function is @safe (although this would give less control over individual parameters)

---
void func2(int[] arr)  // or @trusted
{
    arr ~= 1;               // OK, not @safe or @trusted
    arr.length += 1;        // OK, not @safe or @trusted
    auto slice = arr[0..2];
    slice.length += 1;      // NG, local restriction (1.)
}

void func3(int[] arr) @safe
{
    arr ~= 1;               // NG, not allowed in @safe func
    arr.length += 1;        // NG, not allowed in @safe func
    auto slice = arr[0..2];
    slice.length += 1;      // NG, local restriction (1.)
}
---

Of course ideally a full 1. would be better but then this requires complex DFA, which DMD doesn't know how to do yet, unless I'm not well informed.

On Monday, November 11, 2019 2:34:39 PM MST Walter Bright via Digitalmars-d wrote:
> On 11/11/2019 3:26 AM, Nicholas Wilson wrote:
> > The breaking changes are massive and not well argued for. The suggested workaround of
> >
> > 1) a = a ~ b;
> >
> > in place of
> >
> > 2) a ~= b;
> >
> > is functional identical, thus any issues present in 2 are also present in 1.
> The point is making it obvious that the GC is being used, i.e. it is intentional behavior.

Both ~ and ~= use the GC, and it should be obvious that they do, since they can't work if they don't. Using ~ isn't any clearer than ~=, and ~ has worse performance when a dynamic array is able to grow in-place.

- Jonathan M Davis

On Monday, 11 November 2019 at 21:49:52 UTC, Basile B. wrote:

> 2. When not possible anymore, e.g function parameter, a @slice attribute could be used to apply the same restriction as in 1.

O God, please no! Pretty frankly, writing D cose is starting to became a masterclass of attribute handling: with move semantic, rvalue, live, and now that, I've practically lost the count!

On Monday, 11 November 2019 at 21:41:14 UTC, Walter Bright wrote:
> On 11/11/2019 5:13 AM, Dennis wrote:
>> [...]
>
> That's right. I had overlooked that.
>
>
>> [...]
>
> Imagine these 3 lines are spread out over a large code base.
>
>
>>> [...]
>
> Memory safety cannot be achieved without control over who is the owner of memory.
>
>
>> [...]
>
>   @trusted void myfree(void* p) { free(p); }
>   free(slice.ptr);

That's not @safe code, it's @trusted, it's not mechanically checkable, and all the horrors summoned in a programmers mind can happens here. My impression is that this is not the point at all.

On 11/11/19 4:54 PM, Jonathan M Davis wrote:
> On Monday, November 11, 2019 2:34:39 PM MST Walter Bright via Digitalmars-d
> wrote:
>> On 11/11/2019 3:26 AM, Nicholas Wilson wrote:
>>> The breaking changes are massive and not well argued for.
>>> The suggested workaround of
>>>
>>> 1) a = a ~ b;
>>>
>>> in place of
>>>
>>> 2) a ~= b;
>>>
>>> is functional identical, thus any issues present in 2 are also present
>>> in 1.
>> The point is making it obvious that the GC is being used, i.e. it is
>> intentional behavior.
> 
> Both ~ and ~= use the GC, and it should be obvious that they do, since they
> can't work if they don't. Using ~ isn't any clearer than ~=, and ~ has worse
> performance when a dynamic array is able to grow in-place.

Yes. And the more I see this example, the worse job it does as an example of why this DIP is needed.

In order for the DIP to guarantee the pointer of a slice is still the pointer to memory to be freed, you have to *disable assignment*. Do we want to do that too?

It also must disable popFront, or really any operation except for opIndex.

Here's another example of an error like the item in the DIP, which is totally possible even with the DIP:

@nogc: // just to emphasize
int[] slice = (cast(int*)malloc(10 * int.sizeof))[0 .. 10];
version(bad)
{
  int[1] arr;
  slice = arr[];
}
else version(alsoBad)
{
  slice = slice [1 .. $]; // slice doesn't grow!
}
free(slice.ptr); // Oops;

-Steve