Canonical output ordering for constants.

Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67

Reverted.

Committed: https://github.com/dart-lang/sdk/commit/ae0da7de9cbeb22bc09e2136b629277438ae537a

[sra1, 2015-12-05 00:19:57]

Patchset #1 (id:1) has been deleted

[sra1, 2015-12-05 00:32:11]

Patchset #1 (id:20001) has been deleted

[sra1, 2015-12-05 00:37:15]

Patchset #1 (id:40001) has been deleted

[sra1, 2015-12-07 18:03:17]

Description was changed from

==========
Canonical output ordering for constants.
==========

to

==========
Canonical output ordering for constants.

ix for: http://dartbug.com/24920
==========

[sra1, 2015-12-07 18:03:17]

sra@google.com changed reviewers:
+ sigmund@google.com

[sra1, 2015-12-07 18:03:27]

Description was changed from

==========
Canonical output ordering for constants.

ix for: http://dartbug.com/24920
==========

to

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920
==========

[Siggi Cherem (dart-lang), 2015-12-09 00:36:23]

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/constant_ordering.dart (right):

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:42: static int
compareLists(int compare(a, b), List a, List b) {
nit/idea: maybe make `compare` optional and default to compareValues, since
that's the most common use case you have below.

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:62: //int r =
a.name.compareTo(b.name);
delete commented code

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:97: return
aInt.compareTo(bInt);
can't believe `bool` is not comparable! :(

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:109: int r =
compareLists(compare, a.entries, b.entries);
I couldn't find the definition of `compare`, should this be `compareValues`?
(same at line 115 and 117 in visit Map?)

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:147: int
visitSynthetic(SyntheticConstantValue a, SyntheticConstantValue b) {
seems like there are multiple possible values here, shouldn't we sort these too?
(first by kind from the SyntheticConstantKind enum, then by the payload?)

[sra1, 2015-12-09 23:15:24]

PTAL

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_emitter/constant_ordering.dart (right):

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:42: static int
compareLists(int compare(a, b), List a, List b) {
On 2015/12/09 00:36:23, Siggi Cherem (dart-lang) wrote:
> nit/idea: maybe make `compare` optional and default to compareValues, since
> that's the most common use case you have below.

But then it has to go last, and if we need to make the visitor stateful to
effect a different order it will not be a constant.  IMO not worth it for 3 call
sites.

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:62: //int r =
a.name.compareTo(b.name);
On 2015/12/09 00:36:23, Siggi Cherem (dart-lang) wrote:
> delete commented code 

Done.

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:97: return
aInt.compareTo(bInt);
On 2015/12/09 00:36:23, Siggi Cherem (dart-lang) wrote:
> can't believe `bool` is not comparable! :(

Acknowledged.

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:109: int r =
compareLists(compare, a.entries, b.entries);
On 2015/12/09 00:36:23, Siggi Cherem (dart-lang) wrote:
> I couldn't find the definition of `compare`, should this be `compareValues`?
> (same at line 115 and 117 in visit Map?)

Thanks, a missed last-minute rename not exercised by swarm.

https://codereview.chromium.org/1491413008/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_emitter/constant_ordering.dart:147: int
visitSynthetic(SyntheticConstantValue a, SyntheticConstantValue b) {
On 2015/12/09 00:36:23, Siggi Cherem (dart-lang) wrote:
> seems like there are multiple possible values here, shouldn't we sort these
too?
> (first by kind from the SyntheticConstantKind enum, then by the payload?)

The payload is completely abstract.
Lucky for us these synthetic constants are never top-level named constants, and
they occur embedded in a few big lists.

[Siggi Cherem (dart-lang), 2015-12-10 00:48:44]

lgtm

https://codereview.chromium.org/1491413008/diff/80001/tests/compiler/dart2js_...
File tests/compiler/dart2js_extra/many_constants_test.dart (right):

https://codereview.chromium.org/1491413008/diff/80001/tests/compiler/dart2js_...
tests/compiler/dart2js_extra/many_constants_test.dart:61: /*
delete commented code?

[sra1, 2015-12-10 01:06:06]

Description was changed from

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920
==========

to

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67
==========

[sra1, 2015-12-10 01:06:07]

Committed patchset #3 (id:100001) manually as
d15e5d519261d01617541e8c8c62991ecd7ece67 (presubmit successful).

[sra1, 2015-12-10 01:26:31]

Description was changed from

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67
==========

to

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67

Reverted.
==========

[sra1, 2015-12-10 18:32:30]

Description was changed from

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67

Reverted.
==========

to

==========
Canonical output ordering for constants.

Fix for: http://dartbug.com/24920

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d15e5d519261d01617541e8c8c62991ecd7ece67

Reverted.

Committed:
https://github.com/dart-lang/sdk/commit/ae0da7de9cbeb22bc09e2136b629277438ae537a
==========

[sra1, 2015-12-10 18:32:33]

Committed patchset #4 (id:120001) manually as
ae0da7de9cbeb22bc09e2136b629277438ae537a (presubmit successful).