Clean up operator names.

Clean up operator names.

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

[ahe, 2012-10-16 21:24:46]

Hi Florian,

I would appreciate your thoughts on this.

Cheers,
Peter

[floitsch, 2012-10-17 12:13:46]

I'm concerned that JS will run slower, but otherwise LGTM.
You could also modify the namer so it doesn't append the arity for operators,
but that's maybe for another CL.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/closure.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/closure.dart:489: String value =
name.stringValue;
I would just ask if the element is an operator, and if yes, return "". It's not
as if "5", would help that much.
Otherwise I would use names that are more telling ("add", "subtract", ...).
These are only used when operators create closures which shouldn't be a common
case.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/closure.dart:557: if (surroundingName != "")
parts = parts.prepend(surroundingName);
NYC: no need to do this test.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/ssa/builder.dart:2576: const
SourceString('=='), 1);
No need to change, but ok.

[erikcorry, 2012-10-17 12:46:39]

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/elements/elements.dart:1780: return op;
Having property names that are not valid identifiers will cause V8 to transition
the object to dictionary mode very eagerly.  I don't think we want to do that
because it is much slower.

[ahe, 2012-11-12 13:22:09]

PTAL

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/closure.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/closure.dart:489: String value =
name.stringValue;
On 2012/10/17 12:13:47, floitsch wrote:
> I would just ask if the element is an operator, and if yes, return "". It's
not
> as if "5", would help that much.
> Otherwise I would use names that are more telling ("add", "subtract", ...).
> These are only used when operators create closures which shouldn't be a common
> case.

I'm using more telling names now.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/closure.dart:557: if (surroundingName != "")
parts = parts.prepend(surroundingName);
On 2012/10/17 12:13:47, floitsch wrote:
> NYC: no need to do this test.

Done.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/elements/elements.dart:1780: return op;
On 2012/10/17 12:46:39, erikcorry wrote:
> Having property names that are not valid identifiers will cause V8 to
transition
> the object to dictionary mode very eagerly.  I don't think we want to do that
> because it is much slower.

Done.

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
File dart/lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10905211/diff/3002/dart/lib/compiler/i...
dart/lib/compiler/implementation/ssa/builder.dart:2576: const
SourceString('=='), 1);
On 2012/10/17 12:13:47, floitsch wrote:
> No need to change, but ok.

I removed the field, so I do need to change it.

[ahe, 2012-11-12 13:22:38]

FWIW, I have verified that DeltaBlue and Richards aren't suffering any
performance degradations.

[ahe, 2012-11-12 13:30:38]

Added a test case.

[ngeoffray, 2012-11-13 11:56:19]

LGTM

http://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart
(right):

http://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart:1808: *
For non-operator names, this metod just returns its input.
Should this method be named mapPotentialOperatorNameToIdentifier ?

http://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart:1819: //
means that the resulting closure name will not start with a
what closure? Do you mean member?

http://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart
(right):

http://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:1618: if
(string === '!'   || string === '&&'  || string == '||' ||
Use identical instead.

[ahe, 2012-11-15 07:00:33]

Thank you all for your comments and suggestions!

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart
(right):

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart:1808: *
For non-operator names, this metod just returns its input.
On 2012/11/13 11:56:19, ngeoffray wrote:
> Should this method be named mapPotentialOperatorNameToIdentifier ?

This method really maps an arbitrary method name to a valid Dart identifier.  I
can rename it to methodNameToIdentifier, but that doesn't feel like an
improvement.

Let me know what you think.

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart:1819: //
means that the resulting closure name will not start with a
On 2012/11/13 11:56:19, ngeoffray wrote:
> what closure? Do you mean member?

This comment is a cut-and-paste error.  I have removed the comment.

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart
(right):

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/resolution/members.dart:1618: if
(string === '!'   || string === '&&'  || string == '||' ||
On 2012/11/13 11:56:19, ngeoffray wrote:
> Use identical instead.

Done.

[ngeoffray, 2012-11-15 11:15:58]

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
File dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart
(right):

https://codereview.chromium.org/10905211/diff/17003/dart/sdk/lib/_internal/co...
dart/sdk/lib/_internal/compiler/implementation/elements/elements.dart:1808: *
For non-operator names, this metod just returns its input.
On 2012/11/15 07:00:33, ahe wrote:
> On 2012/11/13 11:56:19, ngeoffray wrote:
> > Should this method be named mapPotentialOperatorNameToIdentifier ?
> 
> This method really maps an arbitrary method name to a valid Dart identifier. 
I
> can rename it to methodNameToIdentifier, but that doesn't feel like an
> improvement.
> 
> Let me know what you think.

I find the use of operator in the name of this method confusing. So I'd prefer
your suggestion of using methodNameToIdentifier instead of
operatorNameToIdentifier.