Add internal operation that converts a growable list to a fixed list.

Add internal operation that converts a growable list to a fixed list.

The conversion breaks the original list, but doesn't copy data.
Currently only used in List.from(Iterable).

BUG= http://dartbug.com/9458
R=floitsch@google.com, iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=32896

[srdjan, 2014-02-05 16:39:59]

DBC

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/growable_array.c...
runtime/lib/growable_array.cc:98: return Array::MakeArray(array);
Maybe use GET_NON_NULL_NATIVE_ARGUMENT instead of CheckedHandle. The difference
is that the former throws an exception if argument is not GrowableObjectArray,
while the later is a little faster but crashes the VM in such case. Typically,
CheckedHandle is used only on receiver when it is known.

[floitsch, 2014-02-06 16:38:39]

LGTM.

https://codereview.chromium.org/134893003/diff/1/sdk/lib/_internal/lib/intern...
File sdk/lib/_internal/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/1/sdk/lib/_internal/lib/intern...
sdk/lib/_internal/lib/internal_patch.dart:18: JS('void', r'#.fixed$length = #',
growableList, true);
use the static method in jsArray.

[Ivan Posva, 2014-02-06 16:56:59]

-Ivan

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/growable_array.c...
runtime/lib/growable_array.cc:98: return Array::MakeArray(array);
On 2014/02/05 16:39:59, srdjan wrote:
> Maybe use GET_NON_NULL_NATIVE_ARGUMENT instead of CheckedHandle. The
difference
> is that the former throws an exception if argument is not GrowableObjectArray,
> while the later is a little faster but crashes the VM in such case. Typically,
> CheckedHandle is used only on receiver when it is known.

What Srdjan said. You are not guaranteed that the value passed here is correct
(which is only considered to be the case for receivers) and thus you need to use
the throwing variant of the handle creation.

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/internal_patch.d...
runtime/lib/internal_patch.dart:9: class _StaticClass {
Why is it necessary to jump through multiple hoops here?

https://codereview.chromium.org/134893003/diff/1/runtime/vm/bootstrap_natives.h
File runtime/vm/bootstrap_natives.h (right):

https://codereview.chromium.org/134893003/diff/1/runtime/vm/bootstrap_natives...
runtime/vm/bootstrap_natives.h:339: V(GrowableList_toFixedLength, 1)            
                                \
The native is not on the class GrowableList, maybe it should be?

https://codereview.chromium.org/134893003/diff/1/sdk/lib/internal/list.dart
File sdk/lib/internal/list.dart (right):

https://codereview.chromium.org/134893003/diff/1/sdk/lib/internal/list.dart#n...
sdk/lib/internal/list.dart:309: external List makeListFixedLength(List
growableList);
Why is this not in the core library as a private within List?

[Lasse Reichstein Nielsen, 2014-02-07 12:55:24]

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/internal_patch.d...
runtime/lib/internal_patch.dart:9: class _StaticClass {
I had problems making the top-level patch function be a native function.
Making it forward to a class static method worked.
If there is a simpler way, I'd love to make it simpler!

https://codereview.chromium.org/134893003/diff/1/runtime/vm/bootstrap_natives.h
File runtime/vm/bootstrap_natives.h (right):

https://codereview.chromium.org/134893003/diff/1/runtime/vm/bootstrap_natives...
runtime/vm/bootstrap_natives.h:339: V(GrowableList_toFixedLength, 1)            
                                \
I don't intend to make the method public. It behaves differently wrt. the
growable array in the VM and dart2js, and that is fine because we only use it in
cases where the growable array is discarded.
If we exposed it to users, we would want it to behave consistent behavior.

Can I put it on GrowableList and make it visible in other internal libraries
(like dart:_internal), but not to user libraries?

https://codereview.chromium.org/134893003/diff/1/sdk/lib/_internal/lib/intern...
File sdk/lib/_internal/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/1/sdk/lib/_internal/lib/intern...
sdk/lib/_internal/lib/internal_patch.dart:18: JS('void', r'#.fixed$length = #',
growableList, true);
On 2014/02/06 16:38:39, floitsch wrote:
> use the static method in jsArray.

Done.

https://codereview.chromium.org/134893003/diff/1/sdk/lib/internal/list.dart
File sdk/lib/internal/list.dart (right):

https://codereview.chromium.org/134893003/diff/1/sdk/lib/internal/list.dart#n...
sdk/lib/internal/list.dart:309: external List makeListFixedLength(List
growableList);
Because I expect to call it from other libraries (e.g., from dart:collection).

[floitsch, 2014-02-07 15:13:13]

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/internal_patch.d...
runtime/lib/internal_patch.dart:9: class _StaticClass {
On 2014/02/07 12:55:25, Lasse Reichstein Nielsen wrote:
> I had problems making the top-level patch function be a native function.
> Making it forward to a class static method worked.
> If there is a simpler way, I'd love to make it simpler!

I guess it's issue 6997.

https://codereview.chromium.org/134893003/diff/1/sdk/lib/core/list.dart
File sdk/lib/core/list.dart (right):

https://codereview.chromium.org/134893003/diff/1/sdk/lib/core/list.dart#newco...
sdk/lib/core/list.dart:95: for (E e in other) {
I think it would also be more efficient, if you used the efficientLength to set
the length immediately, and copy the elements one by one.
But that's another CL...

[Lasse Reichstein Nielsen, 2014-02-10 09:01:53]

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

https://codereview.chromium.org/134893003/diff/1/runtime/lib/growable_array.c...
runtime/lib/growable_array.cc:98: return Array::MakeArray(array);
On 2014/02/06 16:57:00, Ivan Posva wrote:
> On 2014/02/05 16:39:59, srdjan wrote:
> > Maybe use GET_NON_NULL_NATIVE_ARGUMENT instead of CheckedHandle. The
> difference
> > is that the former throws an exception if argument is not
GrowableObjectArray,
> > while the later is a little faster but crashes the VM in such case.
Typically,
> > CheckedHandle is used only on receiver when it is known.
> 
> What Srdjan said. You are not guaranteed that the value passed here is correct
> (which is only considered to be the case for receivers) and thus you need to
use
> the throwing variant of the handle creation.

Done.

[Lasse Reichstein Nielsen, 2014-02-10 09:18:02]

PTAL

[srdjan, 2014-02-10 10:08:15]

https://codereview.chromium.org/134893003/diff/160001/runtime/lib/internal_pa...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/160001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:12: native "GrowableList_toFixedLength";
We have (mostly) standardized native names. In this case it should be called
"StaticClass_makeListFixedLength".

[Lasse Reichstein Nielsen, 2014-02-10 11:42:23]

https://codereview.chromium.org/134893003/diff/160001/runtime/lib/internal_pa...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/160001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:12: native "GrowableList_toFixedLength";
Done.

[floitsch, 2014-02-10 13:25:49]

dart2js still LGTM.

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/growable_ar...
File runtime/lib/growable_array.cc (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/growable_ar...
runtime/lib/growable_array.cc:95: DEFINE_NATIVE_ENTRY(StaticClass_toFixedLength,
1) {
StaticClass_makeListFixedLength

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:10: class _StaticClass {
You should probably also rename this class, so that the API name is not so
strange.

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:11: static List makeListFixedLength(List
growableList)
Add comment, explaining what this does.
In particular it must explain that the returned list can be either the input or
a new list (in which case the old list is destroyed and must not be reused).

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:12: native "StaticClass_toFixedLength";
indentation ?

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:12: native "StaticClass_toFixedLength";
StaticClass_toFixedLength

https://codereview.chromium.org/134893003/diff/210001/runtime/vm/bootstrap_na...
File runtime/vm/bootstrap_natives.h (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/vm/bootstrap_na...
runtime/vm/bootstrap_natives.h:340: V(StaticClass_toFixedLength, 1)             
                               \
"\" is not aligned.

[Ivan Posva, 2014-02-11 04:14:26]

-Ivan

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:5: patch List makeListFixedLength(List
growableList)
I am really wondering why you have to go through a special static class to patch
a top-level function with a native function. I was under the strong impression
that we had done that in other places before and that it should just work. If
not please let us know as this is something we should address.

[Lasse Reichstein Nielsen, 2014-02-11 06:50:03]

> I am really wondering why you have to go through a special static class to
patch
> a top-level function with a native function. I was under the strong impression
> that we had done that in other places before and that it should just work. If
> not please let us know as this is something we should address.

ACK. It "didn't work" when I originally wrote the patch as:
----
patch List makeListFixedLength(List growableList) native
"StaticClass_toFixedLength";
----
It does work now, so I probably did something else wrong at the time.

That's excellent. There's another "VmWorkaround" class I want to remove too. :)

What is the naming protocol for natives that are toplevel? (since they can't be
"ClassName_MethodName"?)

[Lasse Reichstein Nielsen, 2014-02-11 06:50:58]

On 2014/02/11 06:50:03, Lasse Reichstein Nielsen wrote:
> What is the naming protocol for natives that are toplevel? (since they can't
be
> "ClassName_MethodName"?)

Nevermind: Found "Math_sin" as example.

[Lasse Reichstein Nielsen, 2014-02-11 07:08:47]

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/growable_ar...
File runtime/lib/growable_array.cc (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/growable_ar...
runtime/lib/growable_array.cc:95: DEFINE_NATIVE_ENTRY(StaticClass_toFixedLength,
1) {
Now "Internal_makeListFixedLength".

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
File runtime/lib/internal_patch.dart (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:10: class _StaticClass {
class removed.

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:11: static List makeListFixedLength(List
growableList)
Comment added on external declaration.

https://codereview.chromium.org/134893003/diff/210001/runtime/lib/internal_pa...
runtime/lib/internal_patch.dart:12: native "StaticClass_toFixedLength";
All gone.

https://codereview.chromium.org/134893003/diff/210001/runtime/vm/bootstrap_na...
File runtime/vm/bootstrap_natives.h (right):

https://codereview.chromium.org/134893003/diff/210001/runtime/vm/bootstrap_na...
runtime/vm/bootstrap_natives.h:340: V(StaticClass_toFixedLength, 1)             
                               \
On 2014/02/10 13:25:49, floitsch wrote:
> "\" is not aligned.

Done.

[Lasse Reichstein Nielsen, 2014-02-11 07:10:12]

https://codereview.chromium.org/134893003/diff/320001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/134893003/diff/320001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5351: native_name.ToCString(), func.NumParameters());
I added this while debugging, but kept it because I thought it was useful. If
you want it removed again, just say so.

[Lasse Reichstein Nielsen, 2014-02-11 07:10:13]

https://codereview.chromium.org/134893003/diff/320001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/134893003/diff/320001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:5351: native_name.ToCString(), func.NumParameters());
I added this while debugging, but kept it because I thought it was useful. If
you want it removed again, just say so.

[Lasse Reichstein Nielsen, 2014-02-18 10:05:06]

@iposva,srdjan: PTAL

[Ivan Posva, 2014-02-20 01:18:30]

LGTM -ip

[Lasse Reichstein Nielsen, 2014-02-21 13:15:26]

Committed patchset #4 manually as r32896 (presubmit successful).