Improve List implementation.

Improve List implementation.

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

[ahe, 2012-02-20 23:37:38]

ping

[kasperl, 2012-02-21 07:16:43]

LGTM.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.dart
File dart/frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.d...
dart/frog/leg/emitter.dart:336: addInheritFunctionIfNecessary(buffer);
Maybe add a comment here that explains that you're making the closure class
extend Object.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.d...
dart/frog/leg/emitter.dart:340: buffer.add('${inheritsName}($isolateAccess,
$superName);\n');
Less indentation.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:328: builtin$isEmpty$0(var receiver) {
Want to remove var here too?

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:431: iae(argument) {
Maybe make it clear (through a comment at least) that these methods are called
from generated code for user functions.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/mocki...
File dart/frog/leg/lib/mockimpl.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/mocki...
dart/frog/leg/lib/mockimpl.dart:492: Iterator<E> iterator = other.iterator();
Wouldn't it be simpler with:

  for (E element in other) {
    result.add(element);
  }

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:1303: if (methodInterceptionEnabled) {
Would it make sense to move the methodInterceptionEnabled flag to the
interceptors object?

[ngeoffray, 2012-02-21 10:41:45]

DBC

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:443: var iterator = collection.iterator();
Why not using for in?

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:520: if (!isJSArray(receiver)) {
It would look consistent to have this check inverted.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:557: return UNINTERCEPTED(receiver.last());
Ditto for consistency,

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:586: return
UNINTERCEPTED(receiver.removeRange(start, length));
Ditto.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:1303: if (methodInterceptionEnabled) {
On 2012/02/21 07:16:43, kasperl wrote:
> Would it make sense to move the methodInterceptionEnabled flag to the
> interceptors object?

I decided to put it on the compiler because the interceptors object felt like
something that does not have state.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
File dart/frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
dart/frog/leg/ssa/nodes.dart:1095: && inputs.length == 2) {
We could have a boolean setter in this class. It's not obvious that you're
looking for a getter here.

[ahe, 2012-02-21 23:03:25]

Hi Kasper and Nicolas,

Thank you for all your suggestions and comments.

Cheers,
Peter

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.dart
File dart/frog/leg/emitter.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.d...
dart/frog/leg/emitter.dart:336: addInheritFunctionIfNecessary(buffer);
On 2012/02/21 07:16:43, kasperl wrote:
> Maybe add a comment here that explains that you're making the closure class
> extend Object.

Done.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/emitter.d...
dart/frog/leg/emitter.dart:340: buffer.add('${inheritsName}($isolateAccess,
$superName);\n');
On 2012/02/21 07:16:43, kasperl wrote:
> Less indentation.

Done.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
File dart/frog/leg/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:328: builtin$isEmpty$0(var receiver) {
On 2012/02/21 07:16:43, kasperl wrote:
> Want to remove var here too?

Done.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:431: iae(argument) {
On 2012/02/21 07:16:43, kasperl wrote:
> Maybe make it clear (through a comment at least) that these methods are called
> from generated code for user functions.

Done.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:443: var iterator = collection.iterator();
On 2012/02/21 10:41:45, ngeoffray wrote:
> Why not using for in?

Because I thought it wasn't implemented and because I have seen it cause
problems in bailout code. I'll add a TODO.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/js_he...
dart/frog/leg/lib/js_helper.dart:520: if (!isJSArray(receiver)) {
On 2012/02/21 10:41:45, ngeoffray wrote:
> It would look consistent to have this check inverted.

I'll do this in a follow-up CL. I have 17 CL building on top of this, so I don't
want to make the merging harder than necessary.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/mocki...
File dart/frog/leg/lib/mockimpl.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/lib/mocki...
dart/frog/leg/lib/mockimpl.dart:492: Iterator<E> iterator = other.iterator();
On 2012/02/21 07:16:43, kasperl wrote:
> Wouldn't it be simpler with:
> 
>   for (E element in other) {
>     result.add(element);
>   }

Yes. I'll add a todo because for-in and bailout is a dangerous cocktail right
now.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:1303: if (methodInterceptionEnabled) {
On 2012/02/21 10:41:45, ngeoffray wrote:
> On 2012/02/21 07:16:43, kasperl wrote:
> > Would it make sense to move the methodInterceptionEnabled flag to the
> > interceptors object?
> 
> I decided to put it on the compiler because the interceptors object felt like
> something that does not have state.

I'll stay out of this discussion. It seems to me that this flag is temporary.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
File dart/frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
dart/frog/leg/ssa/nodes.dart:1095: && inputs.length == 2) {
On 2012/02/21 10:41:45, ngeoffray wrote:
> We could have a boolean setter in this class. It's not obvious that you're
> looking for a getter here.

There is a boolean "getter" which I can use.

[ngeoffray, 2012-02-22 08:52:04]

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:1303: if (methodInterceptionEnabled) {
On 2012/02/21 23:03:25, ahe wrote:
> On 2012/02/21 10:41:45, ngeoffray wrote:
> > On 2012/02/21 07:16:43, kasperl wrote:
> > > Would it make sense to move the methodInterceptionEnabled flag to the
> > > interceptors object?
> > 
> > I decided to put it on the compiler because the interceptors object felt
like
> > something that does not have state.
> 
> I'll stay out of this discussion. It seems to me that this flag is temporary.

Why do you think it's temporary? The flag is for implementing UNINTERCEPTED.

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
File dart/frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/nodes...
dart/frog/leg/ssa/nodes.dart:1095: && inputs.length == 2) {
On 2012/02/21 23:03:25, ahe wrote:
> On 2012/02/21 10:41:45, ngeoffray wrote:
> > We could have a boolean setter in this class. It's not obvious that you're
> > looking for a getter here.
> 
> There is a boolean "getter" which I can use.

Yes, but I think you're setting it to true in the builder even for a setter.
Setting it to false, or an extra boolean, both work for me.

[ahe, 2012-02-22 08:58:30]

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
File dart/frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9355031/diff/1/dart/frog/leg/ssa/build...
dart/frog/leg/ssa/builder.dart:1303: if (methodInterceptionEnabled) {
On 2012/02/22 08:52:04, ngeoffray wrote:
> On 2012/02/21 23:03:25, ahe wrote:
> > On 2012/02/21 10:41:45, ngeoffray wrote:
> > > On 2012/02/21 07:16:43, kasperl wrote:
> > > > Would it make sense to move the methodInterceptionEnabled flag to the
> > > > interceptors object?
> > > 
> > > I decided to put it on the compiler because the interceptors object felt
> like
> > > something that does not have state.
> > 
> > I'll stay out of this discussion. It seems to me that this flag is
temporary.
> 
> Why do you think it's temporary? The flag is for implementing UNINTERCEPTED.

I see. Another reason for me to stay out of this discussion. I only have a
cursory understanding of this code :-)