Fix this longstanding bug that luckily did not bite us yet: Use Class.prototype.foo$bailout.call(th…

Fix this longstanding bug that luckily did not bite us yet: Use Class.prototype.foo$bailout.call(this, ...) to call a bailout method instead of this.foo$bailout to avoid executing the bailout method of a subclass.
Committed: https://code.google.com/p/dart/source/detail?r=15008

[ahe, 2012-11-15 11:46:23]

Ouch, nice catch.

Would it be better to change the name of the bailout method if it can conflict? 
Presumably that will be more compact.

[ahe, 2012-11-15 11:50:51]

A few comments on the test.  I haven't looked at the other file.

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.dart
File tests/language/bailout_test.dart (right):

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.d...
tests/language/bailout_test.dart:21: Expect.fail('Should never reach B.foo');
Is there are a reason for not just throwing an exception?

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.d...
tests/language/bailout_test.dart:45: Expect.equals(reachedAfoo.value, 1);
Swap order of arguments.  The method is declared as:

  static void equals(var expected, var actual, [String reason = null]) {

[karlklose, 2012-11-15 12:19:06]

LGTM.

[ngeoffray, 2012-11-15 12:26:11]

Thanks Karl and Peter. I have updated the CL with a version that implements
Peter's suggestion of avoiding conflicting names. Let me know which version you
prefer.

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.dart
File tests/language/bailout_test.dart (right):

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.d...
tests/language/bailout_test.dart:21: Expect.fail('Should never reach B.foo');
On 2012/11/15 11:50:51, ahe wrote:
> Is there are a reason for not just throwing an exception?

Not sure, I'm using what other tests seem to be doing.

https://codereview.chromium.org/11416007/diff/1/tests/language/bailout_test.d...
tests/language/bailout_test.dart:45: Expect.equals(reachedAfoo.value, 1);
On 2012/11/15 11:50:51, ahe wrote:
> Swap order of arguments.  The method is declared as:
> 
>   static void equals(var expected, var actual, [String reason = null]) {

Done.

[karlklose, 2012-11-15 14:59:29]

Nicolas, something went wrong with your last two patch sets.

[ngeoffray, 2012-11-15 15:36:10]

On 2012/11/15 14:59:29, karlklose wrote:
> Nicolas, something went wrong with your last two patch sets.

Ah indeed, forgot to add namer.dart. Done.

[karlklose, 2012-11-16 09:31:17]

LGTM!

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:33: /// A cache
of names used for bailout methods. We make sure two
I think you should /**...*/ here.

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:34: /// bailout
methods cannot have the same name because if one is
I find "because if one is defined in a class that is a subclass of the other, we
would resolve to the wrong bailout method at runtime." hard to understand. How
about "because if the two bailout functions are in a class and its subclass, we
would call the wrong method at runtime."

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:56: *
[STATIC_CLOSURE_NAME_NAME]. */
Not your change, but break before "*/".

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:256: while
(usedBailoutNames.contains(name)) name = '$candidate${i++}';
Should we have a UniqueElementNameGenerator abstraction? ;-)

[ngeoffray, 2012-11-16 10:37:56]

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:33: /// A cache
of names used for bailout methods. We make sure two
On 2012/11/16 09:31:17, karlklose wrote:
> I think you should /**...*/ here.

Done.

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:34: /// bailout
methods cannot have the same name because if one is
On 2012/11/16 09:31:17, karlklose wrote:
> I find "because if one is defined in a class that is a subclass of the other,
we
> would resolve to the wrong bailout method at runtime." hard to understand. How
> about "because if the two bailout functions are in a class and its subclass,
we
> would call the wrong method at runtime."

Done.

https://codereview.chromium.org/11416007/diff/3002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:56: *
[STATIC_CLOSURE_NAME_NAME]. */
On 2012/11/16 09:31:17, karlklose wrote:
> Not your change, but break before "*/".

Done.

[ahe, 2012-11-16 11:26:54]

LGTM, nice and simple!