Support general expressions as keys in literal maps.

Support general expressions as keys in literal maps.

R=ngeoffray@google.com

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

[ngeoffray, 2013-09-13 07:30:14]

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:399: bool
hasNonStringKey = false;
Negating names are confusing. How about 'onlyStringKeys'?

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:404: (key
as dynamic).value == MapConstant.PROTO_PROPERTY) {
I'd prefer avoiding this as, and untype 'key'.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:407: if
(!key.isString()) {
Checking twice here. I suggest:
if (key.isString()) {
  if (key.value == ...) {
    protoValue = 
  } else {
    values.add();
  }
} else {
  values.add();
  hasNonStringKey = true;
}

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:428:
Link<DartType> typeArgument = sourceType.typeArguments;
What is this change?

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/constants.dart:426: if
(hasNonStringKey) {
This seems fishy. Why can't you just add keys? Is that an optimization?

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
File sdk/lib/_internal/lib/constant_map.dart (right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:26: class ConstantStringMap<K, V>
extends ConstantMap<K, V> {
Why do you need K now?

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:36: bool containsKey(K key) {
containsKey takes an Object.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:43: V operator [](K key) {
ditto

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:68: bool containsKey(K key) {
Object

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:73: V operator [](K key) {
Object

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:89: final _jsData;
Add comment on _jsData. That's a List right?

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:91: // We cannot create the backing map
on creation since hashCode interceptors
hashCode is user-defined, and therefore should not be invoked. You need to
create an identity based LinkedHashMap instead of makeLiteralMap.

I think the comment you need to put to explain the lazyness is that
IdentityLinkedHashMap is not defined when constants are created.

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
File tests/language/map_literal6_test.dart (right):

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
tests/language/map_literal6_test.dart:12: const A();
Please add tests with user-defined hashCode.

[Johnni Winther, 2013-09-18 12:37:21]

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:399: bool
hasNonStringKey = false;
On 2013/09/13 07:30:15, ngeoffray wrote:
> Negating names are confusing. How about 'onlyStringKeys'?

Done.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:404: (key
as dynamic).value == MapConstant.PROTO_PROPERTY) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> I'd prefer avoiding this as, and untype 'key'.

Done.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:407: if
(!key.isString()) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> Checking twice here. I suggest:
> if (key.isString()) {
>   if (key.value == ...) {
>     protoValue = 
>   } else {
>     values.add();
>   }
> } else {
>   values.add();
>   hasNonStringKey = true;
> }

Completely rewritten.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:428:
Link<DartType> typeArgument = sourceType.typeArguments;
On 2013/09/13 07:30:15, ngeoffray wrote:
> What is this change?

ConstantMap<V> implements Map<String, V> (and friends) are changed to
ConstantMap<K, V> implements Map<K, V>.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/constants.dart:426: if
(hasNonStringKey) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> This seems fishy. Why can't you just add keys? Is that an optimization?

If I just add [keys], we will create an unused constant for the list of key.
Added a comment.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
File sdk/lib/_internal/lib/constant_map.dart (right):

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:26: class ConstantStringMap<K, V>
extends ConstantMap<K, V> {
On 2013/09/13 07:30:15, ngeoffray wrote:
> Why do you need K now?

If you write {"0": 0} K must be dynamic and not String.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:36: bool containsKey(K key) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> containsKey takes an Object.

Yay!

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:43: V operator [](K key) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> ditto

ditto

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:68: bool containsKey(K key) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> Object

Done.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:73: V operator [](K key) {
On 2013/09/13 07:30:15, ngeoffray wrote:
> Object

Done.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:89: final _jsData;
On 2013/09/13 07:30:15, ngeoffray wrote:
> Add comment on _jsData. That's a List right?

Done.

https://codereview.chromium.org/22909056/diff/6001/sdk/lib/_internal/lib/cons...
sdk/lib/_internal/lib/constant_map.dart:91: // We cannot create the backing map
on creation since hashCode interceptors
On 2013/09/13 07:30:15, ngeoffray wrote:
> hashCode is user-defined, and therefore should not be invoked. You need to
> create an identity based LinkedHashMap instead of makeLiteralMap.
> 
> I think the comment you need to put to explain the lazyness is that
> IdentityLinkedHashMap is not defined when constants are created.

Done.

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
File tests/language/map_literal6_test.dart (right):

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
tests/language/map_literal6_test.dart:12: const A();
On 2013/09/13 07:30:15, ngeoffray wrote:
> Please add tests with user-defined hashCode.

Done.

[ngeoffray, 2013-09-18 14:32:08]

LGTM with comments.

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
File tests/language/map_literal6_test.dart (right):

https://codereview.chromium.org/22909056/diff/6001/tests/language/map_literal...
tests/language/map_literal6_test.dart:12: const A();
On 2013/09/18 12:37:21, Johnni Winther wrote:
> On 2013/09/13 07:30:15, ngeoffray wrote:
> > Please add tests with user-defined hashCode.
> 
> Done.

Where?

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/constants.dart:430: // for the keys.
I don't understand what is be the difference with onlyStringKeys then. It's fine
for onlyStringKeys to generate an unused list constant?

I though that keys would be the jsData.

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
File sdk/lib/_internal/lib/constant_map.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
sdk/lib/_internal/lib/constant_map.dart:66: bool containsKey(K key) {
Object

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
sdk/lib/_internal/lib/constant_map.dart:71: V operator [](K key) {
Object

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:1426: !identical(JS('String', 'typeof #',
object), 'object')) {
!identical => !=

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:1441: makeConstantMap(keyValuePairs) {
Where is that being registered?

[Johnni Winther, 2013-09-18 14:53:47]

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/constants.dart:430: // for the keys.
On 2013/09/18 14:32:08, ngeoffray wrote:
> I don't understand what is be the difference with onlyStringKeys then. It's
fine
> for onlyStringKeys to generate an unused list constant?
> 
> I though that keys would be the jsData.

For [:!onlyStringKeys:] jsData is the key-value pairs list generated in the
ConstantEmitter passed to [GeneralConstantMap]. In the case of [onlyStringKeys]
the [keys] list is provided as argument to [ConstantStringMap] in addition to
the key-value pairs list.

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
File sdk/lib/_internal/lib/constant_map.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
sdk/lib/_internal/lib/constant_map.dart:66: bool containsKey(K key) {
On 2013/09/18 14:32:08, ngeoffray wrote:
> Object

Done.

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/con...
sdk/lib/_internal/lib/constant_map.dart:71: V operator [](K key) {
On 2013/09/18 14:32:08, ngeoffray wrote:
> Object

Done.

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:1426: !identical(JS('String', 'typeof #',
object), 'object')) {
On 2013/09/18 14:32:08, ngeoffray wrote:
> !identical => !=

Done.

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_helper.dart:1441: makeConstantMap(keyValuePairs) {
On 2013/09/18 14:32:08, ngeoffray wrote:
> Where is that being registered?

It is called in-code in GeneralConstantMap._getMap().

[Johnni Winther, 2013-09-18 15:00:26]

Committed patchset #5 manually as r27622 (presubmit successful).

[ngeoffray, 2013-09-18 15:04:56]

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/constants.dart (right):

https://codereview.chromium.org/22909056/diff/14001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/constants.dart:430: // for the keys.
On 2013/09/18 14:53:47, Johnni Winther wrote:
> On 2013/09/18 14:32:08, ngeoffray wrote:
> > I don't understand what is be the difference with onlyStringKeys then. It's
> fine
> > for onlyStringKeys to generate an unused list constant?
> > 
> > I though that keys would be the jsData.
> 
> For [:!onlyStringKeys:] jsData is the key-value pairs list generated in the
> ConstantEmitter passed to [GeneralConstantMap]. In the case of
[onlyStringKeys]
> the [keys] list is provided as argument to [ConstantStringMap] in addition to
> the key-value pairs list.

OK, please add that as a comment.