Add Symbol class.

Add Symbol class.

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

[kasperl, 2013-04-11 10:48:58]

LGTM, but you should reach out to Florian and Lasse so they are aware of this
change landing soon.

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
2013

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:16: if (!new
RegExp(r'^[a-zA-Z$][a-zA-Z$0-9]*=?').hasMatch(name)) {
Throw the RegExp into a lazily initialized static final field? Allow non-leading
underscores?

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:18: '"$name" is not an identifier or the
empty String');
I'd change 'the empty String' to 'an empty string'.

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:305: patch
const Symbol(String name) :
this._name as a parameter?

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart
File dart/sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart...
dart/sdk/lib/core/symbol.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
2013

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart...
dart/sdk/lib/core/symbol.dart:16: * optionally followed by [:'=':].
Do we expect to need the = suffix long term?

https://codereview.chromium.org/14142003/diff/1/dart/tests/corelib/symbol_tes...
File dart/tests/corelib/symbol_test.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/tests/corelib/symbol_tes...
dart/tests/corelib/symbol_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
2013

https://codereview.chromium.org/14142003/diff/3001/dart/tests/corelib/symbol_...
File dart/tests/corelib/symbol_test.dart (right):

https://codereview.chromium.org/14142003/diff/3001/dart/tests/corelib/symbol_...
dart/tests/corelib/symbol_test.dart:13: print(x = const Symbol('fisk'));
Maybe turn x into a list of symbols and check more legal identifiers with $, _,
digits in them?

[ahe, 2013-04-11 11:26:06]

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2013/04/11 10:48:58, kasperl wrote:
> 2013

Done.

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:16: if (!new
RegExp(r'^[a-zA-Z$][a-zA-Z$0-9]*=?').hasMatch(name)) {
On 2013/04/11 10:48:58, kasperl wrote:
> Throw the RegExp into a lazily initialized static final field? Allow
non-leading
> underscores?

Done * 2

https://codereview.chromium.org/14142003/diff/1/dart/runtime/lib/symbol_patch...
dart/runtime/lib/symbol_patch.dart:18: '"$name" is not an identifier or the
empty String');
On 2013/04/11 10:48:58, kasperl wrote:
> I'd change 'the empty String' to 'an empty string'.

Done.

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/_internal/compil...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/_internal/compil...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:305: patch
const Symbol(String name) :
On 2013/04/11 10:48:58, kasperl wrote:
> this._name as a parameter?

The signature has to match exactly.

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart
File dart/sdk/lib/core/symbol.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart...
dart/sdk/lib/core/symbol.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2013/04/11 10:48:58, kasperl wrote:
> 2013

Done.

https://codereview.chromium.org/14142003/diff/1/dart/sdk/lib/core/symbol.dart...
dart/sdk/lib/core/symbol.dart:16: * optionally followed by [:'=':].
On 2013/04/11 10:48:58, kasperl wrote:
> Do we expect to need the = suffix long term?

It's in the specification.

https://codereview.chromium.org/14142003/diff/1/dart/tests/corelib/symbol_tes...
File dart/tests/corelib/symbol_test.dart (right):

https://codereview.chromium.org/14142003/diff/1/dart/tests/corelib/symbol_tes...
dart/tests/corelib/symbol_test.dart:1: // Copyright (c) 2012, the Dart project
authors.  Please see the AUTHORS file
On 2013/04/11 10:48:58, kasperl wrote:
> 2013

Done.

[ahe, 2013-04-11 11:32:27]

PTAL, added hashCode and operator==

[kasperl, 2013-04-11 11:37:00]

https://codereview.chromium.org/14142003/diff/8003/dart/runtime/lib/symbol_pa...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14142003/diff/8003/dart/runtime/lib/symbol_pa...
dart/runtime/lib/symbol_patch.dart:28: return _name == _name;
Shouldn't this be checking that other is a Symbol and shouldn't it be _name ==
other._name?

https://codereview.chromium.org/14142003/diff/8003/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14142003/diff/8003/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:309: return
_name == _name;
Check that other is a symbol and that _name == other._name.

[ahe, 2013-04-11 11:47:50]

PTAL

https://codereview.chromium.org/14142003/diff/8003/dart/runtime/lib/symbol_pa...
File dart/runtime/lib/symbol_patch.dart (right):

https://codereview.chromium.org/14142003/diff/8003/dart/runtime/lib/symbol_pa...
dart/runtime/lib/symbol_patch.dart:28: return _name == _name;
On 2013/04/11 11:37:00, kasperl wrote:
> Shouldn't this be checking that other is a Symbol and shouldn't it be _name ==
> other._name?

Done.

https://codereview.chromium.org/14142003/diff/8003/dart/sdk/lib/_internal/com...
File dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart (right):

https://codereview.chromium.org/14142003/diff/8003/dart/sdk/lib/_internal/com...
dart/sdk/lib/_internal/compiler/implementation/lib/core_patch.dart:309: return
_name == _name;
On 2013/04/11 11:37:00, kasperl wrote:
> Check that other is a symbol and that _name == other._name.

Done.

[kasperl, 2013-04-11 11:51:11]

LGTM.

[ahe, 2013-04-11 12:46:32]

Committed patchset #6 manually as r21273 (presubmit successful).

[ahe, 2013-04-11 20:46:38]

Sorry, I forgot to CC Gilad and Ivan.

[Ivan Posva, 2013-04-12 03:34:39]

As this has already been submitted, I'll address my comments myself.

-Ivan

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/corelib...
File dart/tests/corelib/corelib.status (right):

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/corelib...
dart/tests/corelib/corelib.status:12: symbol_test/01: Fail
Fail, OK. See comment in symbol_test.

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/symbol_...
File dart/tests/corelib/symbol_test.dart (right):

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/symbol_...
dart/tests/corelib/symbol_test.dart:12: print(const Symbol(0)); /// 01:
compile-time error
As long as this is not a language feature, I don't think you can expect a
compile-time error here.

[ahe, 2013-04-12 05:53:00]

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/symbol_...
File dart/tests/corelib/symbol_test.dart (right):

https://codereview.chromium.org/14142003/diff/1009/dart/tests/corelib/symbol_...
dart/tests/corelib/symbol_test.dart:12: print(const Symbol(0)); /// 01:
compile-time error
On 2013/04/12 03:34:39, Ivan Posva wrote:
> As long as this is not a language feature, I don't think you can expect a
> compile-time error here.

Actually, it is a bug in the VM: it is a compile-time error if evaluation of a
compile-time constant would raise an exception.  See section 12.1 of the
specification.