Skip spurious constants in deferred computation.

Skip spurious constants in deferred computation.

BUG=
R=sigmund@google.com

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

[Johnni Winther, 2016-05-09 14:20:24]

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

[Siggi Cherem (dart-lang), 2016-05-09 20:03:04]

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
File pkg/compiler/lib/src/constants/expressions.dart (right):

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
pkg/compiler/lib/src/constants/expressions.dart:138: /// if it contains
positional or named references, used to defined constant
to defined => to define?

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
pkg/compiler/lib/src/constants/expressions.dart:140: bool get isPotential =>
true;
should we make this a final field instead? In all the subclasses, we know in the
constructor whether or not it is potential.

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:341: // be added, and some implicit
constant expression that should be
rephrase? (I'm not sure I understood the last phrase)

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:56: deferred/*:
Pass,RuntimeError # Issue 17458
Are these actually flaky? Can we just do "RuntimeError" or maybe use
"SkipByDesign"? Looking at the bug, it seems this is not supported at this time?

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status#newcod...
tests/lib/lib.status:297: async/*deferred*: Pass,RuntimeError # Issue 17458
ditto

[Siggi Cherem (dart-lang), 2016-05-09 20:03:12]

otherwise lgtm

[Johnni Winther, 2016-05-10 07:41:38]

Description was changed from

==========
Skip spurious constants in deferred computation.

BUG=
==========

to

==========
Skip spurious constants in deferred computation.

BUG=
R=sigmund@google.com

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

[Johnni Winther, 2016-05-10 07:41:41]

Committed patchset #2 (id:20001) manually as
24c548f5bd0f91f8f53714fe189ac6a22370ca9a (presubmit successful).

[Johnni Winther, 2016-05-10 09:26:16]

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
File pkg/compiler/lib/src/constants/expressions.dart (right):

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
pkg/compiler/lib/src/constants/expressions.dart:138: /// if it contains
positional or named references, used to defined constant
On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> to defined => to define?

Done.

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/consta...
pkg/compiler/lib/src/constants/expressions.dart:140: bool get isPotential =>
true;
On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> should we make this a final field instead? In all the subclasses, we know in
the
> constructor whether or not it is potential.

I think it'll only be need for asserts, in which case lazy computation is
preferable. Add a TODO with the consideration.

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/1955403002/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:341: // be added, and some implicit
constant expression that should be
On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> rephrase? (I'm not sure I understood the last phrase)

Done.

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:56: deferred/*:
Pass,RuntimeError # Issue 17458
On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> Are these actually flaky? Can we just do "RuntimeError" or maybe use
> "SkipByDesign"? Looking at the bug, it seems this is not supported at this
time?

d8 on Windows doesn't support the properties we use to tell where the script is
located. For that reason, some deferred tests (the positive ones) fail at
runtime, whereas some deferred tests (the negative ones) might not fail or are
expected to fail.

To give better coverage on Windows we still compile the tests (checking
invariants and such) but don't expect to much of the runtime.

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status#newcod...
tests/lib/lib.status:297: async/*deferred*: Pass,RuntimeError # Issue 17458
On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> ditto

ditto!

[Siggi Cherem (dart-lang), 2016-05-10 16:59:09]

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/dart2js_extra.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/dart2js_extra.status:56: deferred/*:
Pass,RuntimeError # Issue 17458
On 2016/05/10 09:26:16, Johnni Winther wrote:
> On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> > Are these actually flaky? Can we just do "RuntimeError" or maybe use
> > "SkipByDesign"? Looking at the bug, it seems this is not supported at this
> time?
> 
> d8 on Windows doesn't support the properties we use to tell where the script
is
> located. For that reason, some deferred tests (the positive ones) fail at
> runtime, whereas some deferred tests (the negative ones) might not fail or are
> expected to fail.
> 
> To give better coverage on Windows we still compile the tests (checking
> invariants and such) but don't expect to much of the runtime.

I see. Two suggestions then:
 - label each test individually: that way each test is either Pass or
RuntimeError, but not both. It's my understanding that the way it is currently
written marks every test as flaky and will make our test infrastructure waste
time retrying the tests a couple times.

 - whenever we have a `RuntimeError`, write in the comment that this at least
expects no `CompileTimeError`.

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/1955403002/diff/1/tests/lib/lib.status#newcod...
tests/lib/lib.status:297: async/*deferred*: Pass,RuntimeError # Issue 17458
On 2016/05/10 09:26:16, Johnni Winther wrote:
> On 2016/05/09 20:03:04, Siggi Cherem (dart-lang) wrote:
> > ditto
> 
> ditto!

re-ditto :-)