Shrink growable arrays

Shrink growable arrays

Fixes #25778

Committed: https://github.com/dart-lang/sdk/commit/5590ba31bebc73a9a807f06fcb44c7c9cc8b8af6

[Cutch, 2016-02-16 23:09:12]

Description was changed from

==========
Shrink growable arrays

BUG=
==========

to

==========
Shrink growable arrays

Fixes #25778
==========

[Cutch, 2016-02-16 23:09:13]

johnmccutchan@google.com changed reviewers:
+ iposva@google.com

[srdjan, 2016-02-16 23:45:29]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2016-02-16 23:45:29]

DBC

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:239: void _shrink(int new_length) {
if (new_length == _capacity) return

[Ivan Posva, 2016-02-16 23:54:54]

LGTMwC

-Ivan

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:121: if (new_length > _capacity) {
Judging from Srdjan's comment we should not intermix new_length and new_capacity
interchangably here.

How about:
void set length(int new_length) {
  var new_capacity = (new_length == 0) ? _kDefaultCapacity : new_length;
  if (new_capacity > capacity) {
    ...
  }

and changing the parameter name in _grow and _shrink to be clearer (e.g.
new_capacity).

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:128: // (null fill + copy).
How about "(null init + copy)" and "(null overwrite)"?

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:131: final bool shouldShrinkToFit = (2 *
new_length) < (length - new_length);
Which would mean here (new_capacity + new_length) < (length - new_length).

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:239: void _shrink(int new_length) {
On 2016/02/16 23:45:29, srdjan wrote:
> if (new_length == _capacity) return

I don't think this would work here as the code above assumes that the data array
will be initialized with nulls automatically. The new call does this implicitly.

[Cutch, 2016-02-17 21:12:54]

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:121: if (new_length > _capacity) {
On 2016/02/16 23:54:54, Ivan Posva wrote:
> Judging from Srdjan's comment we should not intermix new_length and
new_capacity
> interchangably here.
> 
> How about:
> void set length(int new_length) {
>   var new_capacity = (new_length == 0) ? _kDefaultCapacity : new_length;
>   if (new_capacity > capacity) {
>     ...
>   }
> 
> and changing the parameter name in _grow and _shrink to be clearer (e.g.
> new_capacity).

Done.

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:128: // (null fill + copy).
On 2016/02/16 23:54:54, Ivan Posva wrote:
> How about "(null init + copy)" and "(null overwrite)"?

Done.

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:131: final bool shouldShrinkToFit = (2 *
new_length) < (length - new_length);
On 2016/02/16 23:54:54, Ivan Posva wrote:
> Which would mean here (new_capacity + new_length) < (length - new_length).

Done.

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:239: void _shrink(int new_length) {
On 2016/02/16 23:54:54, Ivan Posva wrote:
> On 2016/02/16 23:45:29, srdjan wrote:
> > if (new_length == _capacity) return
> 
> I don't think this would work here as the code above assumes that the data
array
> will be initialized with nulls automatically. The new call does this
implicitly.

Acknowledged.

https://codereview.chromium.org/1701213002/diff/1/runtime/lib/growable_array....
runtime/lib/growable_array.dart:239: void _shrink(int new_length) {
On 2016/02/16 23:45:29, srdjan wrote:
> if (new_length == _capacity) return

Acknowledged.

[Cutch, 2016-02-17 21:20:57]

Description was changed from

==========
Shrink growable arrays

Fixes #25778
==========

to

==========
Shrink growable arrays

Fixes #25778

Committed:
https://github.com/dart-lang/sdk/commit/5590ba31bebc73a9a807f06fcb44c7c9cc8b8af6
==========

[Cutch, 2016-02-17 21:21:00]

Committed patchset #2 (id:20001) manually as
5590ba31bebc73a9a807f06fcb44c7c9cc8b8af6 (presubmit successful).