Support unittest of jumps

Support unittest of jumps

R=sigmund@google.com

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

[Johnni Winther, 2017-08-18 19:05:08]

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

[Siggi Cherem (dart-lang), 2017-08-18 21:20:21]

lgtm

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
File tests/compiler/dart2js/equivalence/id_equivalence.dart (right):

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:85: abstract class
ComputerMixin {
nit: rename to describe what it does and not what it is used for.

This mixin doesn't seem to be computing anything :).. maybe DataRegistryMixin?

To be honest, it might not be worth using a mixin here - consider just adding
the method on both visitors below.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:90: if (id != null &&
value != null) {
note that you already guard agaisnt null id's on all callsites, so we could
probably remove that check.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:96: /// Abstract AST
visitor for computing [Id] data.
nit: reword to: visitor for computing data corresponding to a node or element,
and record it with a generic [Id].

(currently it sounds like the data is about the "id" and not about the
element/node they represent)

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:97: abstract class
AbstractResolvedAstComputer extends ast.Visitor
other naming ideas:
 - AbstractAstDataBuilder and AbstractIrDataBuilder
 - AstDataExtractor/IrDataExtractor


I sort of like extractor :)

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:150: String
computeElementValue(AstElement element);
let's move these two up and add dart doc on them. Since this is what each
equivalence test is supposed to override, correct?

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
File tests/compiler/dart2js/equivalence/id_equivalence_helper.dart (left):

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence_helper.dart:253: abstract
class ComputerMixin {
ha! - I just realized this was a move :(...

comments still apply I guess :)

[Johnni Winther, 2017-08-19 08:53:12]

Description was changed from

==========
Support unittest of jumps
==========

to

==========
Support unittest of jumps

R=sigmund@google.com

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

[Johnni Winther, 2017-08-19 08:53:13]

Committed patchset #2 (id:20001) manually as
0b48a5880e26d10488c43c012a0168c71083a9a7 (presubmit successful).

[Johnni Winther, 2017-08-19 09:30:52]

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
File tests/compiler/dart2js/equivalence/id_equivalence.dart (right):

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:85: abstract class
ComputerMixin {
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> nit: rename to describe what it does and not what it is used for.
> 
> This mixin doesn't seem to be computing anything :).. maybe DataRegistryMixin?
> 
> To be honest, it might not be worth using a mixin here - consider just adding
> the method on both visitors below.

Mixin removed

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:90: if (id != null &&
value != null) {
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> note that you already guard agaisnt null id's on all callsites, so we could
> probably remove that check.

Done.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:96: /// Abstract AST
visitor for computing [Id] data.
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> nit: reword to: visitor for computing data corresponding to a node or element,
> and record it with a generic [Id].
> 
> (currently it sounds like the data is about the "id" and not about the
> element/node they represent)

Done.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:97: abstract class
AbstractResolvedAstComputer extends ast.Visitor
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> other naming ideas:
>  - AbstractAstDataBuilder and AbstractIrDataBuilder
>  - AstDataExtractor/IrDataExtractor
> 
> 
> I sort of like extractor :)

Extractor it is.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence.dart:150: String
computeElementValue(AstElement element);
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> let's move these two up and add dart doc on them. Since this is what each
> equivalence test is supposed to override, correct?

Done.

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
File tests/compiler/dart2js/equivalence/id_equivalence_helper.dart (left):

https://codereview.chromium.org/3001223002/diff/1/tests/compiler/dart2js/equi...
tests/compiler/dart2js/equivalence/id_equivalence_helper.dart:253: abstract
class ComputerMixin {
On 2017/08/18 21:20:21, Siggi Cherem (dart-lang) wrote:
> ha! - I just realized this was a move :(...
> 
> comments still apply I guess :)

Acknowledged.