Move some string methods in the interceptor, and support for optional parameters in intercepted met…

Move some string methods in the interceptor, and support for optional parameters in intercepted methods.
Committed: https://code.google.com/p/dart/source/detail?r=14881

[floitsch, 2012-11-13 18:20:01]

LGTM.

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
Too brittle. Discuss with Erik how this should be done.

[ahe, 2012-11-13 18:41:26]

LGTM, when you address Florian's comment.

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:397:
JavaScriptBackend backend = compiler.backend;
Looks like this variable is only used once.

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:15: class
JSString implements String {
Shouldn't this class be in its own file?

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:76: return
substringUnchecked(this, startIndex, endIndex);
I think you can inline this method now.

[ngeoffray, 2012-11-14 09:18:43]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
On 2012/11/13 18:20:01, floitsch wrote:
> Too brittle. Discuss with Erik how this should be done.

Note that this is not touched by the minifier. I could use Namer.isolateName
instead.

[ngeoffray, 2012-11-14 09:24:44]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:397:
JavaScriptBackend backend = compiler.backend;
On 2012/11/13 18:41:26, ahe wrote:
> Looks like this variable is only used once.

Yes, but I need the 'cast' to JavaScriptBackend.

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:15: class
JSString implements String {
On 2012/11/13 18:41:26, ahe wrote:
> Shouldn't this class be in its own file?

Will do in a follow-up CL.

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:76: return
substringUnchecked(this, startIndex, endIndex);
On 2012/11/13 18:41:26, ahe wrote:
> I think you can inline this method now.

Done.

[floitsch, 2012-11-14 10:06:33]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
On 2012/11/14 09:18:43, ngeoffray wrote:
> On 2012/11/13 18:20:01, floitsch wrote:
> > Too brittle. Discuss with Erik how this should be done.
> 
> Note that this is not touched by the minifier. I could use Namer.isolateName
> instead.

1. It should go through the minifier (eventually).
2. If we change the isolate-name from "$" to something else you cannot be sure
anymore that the element-name will not clash with the isolate-name. Currently
"$" is always escaped by "getValid", but that's more of an accident.

[ngeoffray, 2012-11-14 10:10:58]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
On 2012/11/14 10:06:33, floitsch wrote:
> On 2012/11/14 09:18:43, ngeoffray wrote:
> > On 2012/11/13 18:20:01, floitsch wrote:
> > > Too brittle. Discuss with Erik how this should be done.
> > 
> > Note that this is not touched by the minifier. I could use Namer.isolateName
> > instead.
> 
> 1. It should go through the minifier (eventually).
> 2. If we change the isolate-name from "$" to something else you cannot be sure
> anymore that the element-name will not clash with the isolate-name. Currently
> "$" is always escaped by "getValid", but that's more of an accident.

It doesn't have to be an accident, just an internal convention. If I'm not using
that convention, I think the only option is to go through the list of parameter
names, and make up one that isn't used (eg temp).

Note that I could also do the minifiying myself in this code (using a, b, c, d
as parameter names).

[floitsch, 2012-11-14 10:18:45]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
On 2012/11/14 10:10:58, ngeoffray wrote:
> On 2012/11/14 10:06:33, floitsch wrote:
> > On 2012/11/14 09:18:43, ngeoffray wrote:
> > > On 2012/11/13 18:20:01, floitsch wrote:
> > > > Too brittle. Discuss with Erik how this should be done.
> > > 
> > > Note that this is not touched by the minifier. I could use
Namer.isolateName
> > > instead.
> > 
> > 1. It should go through the minifier (eventually).
> > 2. If we change the isolate-name from "$" to something else you cannot be
sure
> > anymore that the element-name will not clash with the isolate-name.
Currently
> > "$" is always escaped by "getValid", but that's more of an accident.
> 
> It doesn't have to be an accident, just an internal convention. If I'm not
using
> that convention, I think the only option is to go through the list of
parameter
> names, and make up one that isn't used (eg temp).
> 
> Note that I could also do the minifiying myself in this code (using a, b, c, d
> as parameter names).

Yes. there are several possibilities. That's why I propose to talk with Erik.

[Erik Corry, 2012-11-14 11:27:05]

I think this will work for now.  When we start minifying this code we will want
to do something better.

[floitsch, 2012-11-14 12:01:26]

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11366224/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:415: // Use $
to avoid clashes with other parameter names.
On 2012/11/14 10:18:45, floitsch wrote:
> On 2012/11/14 10:10:58, ngeoffray wrote:
> > On 2012/11/14 10:06:33, floitsch wrote:
> > > On 2012/11/14 09:18:43, ngeoffray wrote:
> > > > On 2012/11/13 18:20:01, floitsch wrote:
> > > > > Too brittle. Discuss with Erik how this should be done.
> > > > 
> > > > Note that this is not touched by the minifier. I could use
> Namer.isolateName
> > > > instead.
> > > 
> > > 1. It should go through the minifier (eventually).
> > > 2. If we change the isolate-name from "$" to something else you cannot be
> sure
> > > anymore that the element-name will not clash with the isolate-name.
> Currently
> > > "$" is always escaped by "getValid", but that's more of an accident.
> > 
> > It doesn't have to be an accident, just an internal convention. If I'm not
> using
> > that convention, I think the only option is to go through the list of
> parameter
> > names, and make up one that isn't used (eg temp).
> > 
> > Note that I could also do the minifiying myself in this code (using a, b, c,
d
> > as parameter names).
> 
> Yes. there are several possibilities. That's why I propose to talk with Erik.

As Erik suggests: this should work for now.
Please add a comment, that this works because "JsNames.getValid" below is
guaranteed never to return "$".
So this choice has nothing to do with the fact that the isolate is "$".