Move the handling of operator[] into the new interceptors.

Move the handling of operator[] into the new interceptors.
Committed: https://code.google.com/p/dart/source/detail?r=16032

[kasperl, 2012-11-30 09:23:03]

LGTM.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:149: var temp =
instruction;
How about factoring this condition out into a separate helper method that takes
a 'var' argument? Maybe check for hasTypeGuards as the first thing because it's
a cheap check?

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2627: Selector
selector = elements.getSelector(node);
How about refactoring this? Maybe add a helper that given a selector and a
receiver (and a node) returns an input list that is either empty or contains the
result of invoking the interceptor?

Could be that the helper you really want is something that returns the
HInvokeDynamicMethod in which case you'll need to pass the index to the function
too. Long term, you may need a few variants (depending on the number of
arguments to the operator?) of this kind of helper.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3532: HInstruction
left = new HInvokeDynamicMethod( selector, inputs);
Remove space before selector.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2468: class HIndex
extends HInstruction {
This is awesome.

[ngeoffray, 2012-11-30 13:18:13]

Thanks Kasper.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/bailout.dart (right):

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/bailout.dart:149: var temp =
instruction;
On 2012/11/30 09:23:04, kasperl wrote:
> How about factoring this condition out into a separate helper method that
takes
> a 'var' argument? Maybe check for hasTypeGuards as the first thing because
it's
> a cheap check?

Done.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2627: Selector
selector = elements.getSelector(node);
On 2012/11/30 09:23:04, kasperl wrote:
> How about refactoring this? Maybe add a helper that given a selector and a
> receiver (and a node) returns an input list that is either empty or contains
the
> result of invoking the interceptor?
> 
> Could be that the helper you really want is something that returns the
> HInvokeDynamicMethod in which case you'll need to pass the index to the
function
> too. Long term, you may need a few variants (depending on the number of
> arguments to the operator?) of this kind of helper.

Done.

https://codereview.chromium.org/11348316/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3532: HInstruction
left = new HInvokeDynamicMethod( selector, inputs);
On 2012/11/30 09:23:04, kasperl wrote:
> Remove space before selector.

Done.

[ngeoffray, 2012-12-05 04:58:00]

+sra

Stephen, could you take a look at the native_handler.dart changes? It looks like
the native enqueuer for codegen (which is currently a hack) is doing more things
than it needs. The intent of my changes is to have the nativeEnqueuer also take
care of non-html classes, like a JS array. I don't want the SSA backend do the
same analysis (resolving types inside a foreign JS call) as the native enqueuer.

[ngeoffray, 2012-12-10 13:23:31]

@Stephen: ping

[sra1, 2012-12-10 23:42:21]

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:853: //
[:ioore:] and [:iae:] helpers directly.
backtick, e.g. `ioore`, is more readable that [::],

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (left):

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:402:
super.processNativeClasses(libraries);
Removing this will mess up --disable-native-live-type-analysis, and the various
sets of classes.
The intention is to remove the 'enqueue all resolved classes' hack, so we will
need the native classes to progress through the stages unused -> pending ->
registered.

Please test with --disable-native-live-type-analysis.

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:105:
libraries.forEach(processNativeClassesInLibrary);
After adding all the native classes, you will need to add the classes that you
want to treat like native classes (listClass etc).

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:191:
assert(unusedClasses.contains(classElement));
I don't know why this is not firing for you.

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:314:
enqueueClass(compiler.listClass, 'core type');
Line before this should be unnecessary, since enqueueing the listClass will lead
to registerInstantiatedClass.

[ngeoffray, 2012-12-11 09:30:15]

Thanks Stephen, PTAL

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:853: //
[:ioore:] and [:iae:] helpers directly.
On 2012/12/10 23:42:21, sra1 wrote:
> backtick, e.g. `ioore`, is more readable that [::], 

Done.

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:105:
libraries.forEach(processNativeClassesInLibrary);
On 2012/12/10 23:42:21, sra1 wrote:
> After adding all the native classes, you will need to add the classes that you
> want to treat like native classes (listClass etc).

Done.

https://codereview.chromium.org/11348316/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:314:
enqueueClass(compiler.listClass, 'core type');
On 2012/12/10 23:42:21, sra1 wrote:
> Line before this should be unnecessary, since enqueueing the listClass will
lead
> to registerInstantiatedClass.

Done.

[sra1, 2012-12-12 04:28:02]

lgtm.

Unless this fixes an M2 issue, it might be prudent to submit after M2.

https://codereview.chromium.org/11348316/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11348316/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:117: if
(classElement.isNative() || classElement == compiler.listClass) {
And compile.stringClass?

It would be better to add the class(es) between lines 105/106 than check every
class for ==.

[ngeoffray, 2012-12-12 11:54:56]

I went ahead and submitted. This CL has been standing here for just too long :-)

https://codereview.chromium.org/11348316/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/11348316/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/native_handler.dart:117: if
(classElement.isNative() || classElement == compiler.listClass) {
On 2012/12/12 04:28:02, sra1 wrote:
> And compile.stringClass?
> 
> It would be better to add the class(es) between lines 105/106 than check every
> class for ==.

Done. I also added string, int, double and bool as native classes.