dartk: remove uses of .computeNode

dartk: remove uses of .computeNode

As a result, this changes the order in which declarations appear in the generated output. The new order is more consistent with the textual order in the original file.

Calling .computeNode() is sort of an anti-pattern for using analyzer. This was
however used very sparsely in dartk. The few places where it was used,
computeNode was practically called immediately after resolving the library
element, so I don't expect this to make almost any difference in performance.

R=asgerf@google.com, paulberry@google.com

Committed: https://github.com/dart-lang/kernel/commit/f315ae1c00ee804167483d6a41be2bfe3d48fdc6

[Siggi Cherem (dart-lang), 2016-12-06 23:58:46]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-12-07 00:01:33]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2016-12-07 00:28:09]

sigmund@google.com changed reviewers:
+ asgerf@google.com, paulberry@google.com

[Siggi Cherem (dart-lang), 2016-12-07 00:28:10]

Tested by:
- compiling dart2js with dartk
- running:

 ./tools/test.py -m release -c dartk language

Asger - let me know if there are some additional tests I should run.

[asgerf, 2016-12-07 11:34:00]

LGTM (with the strong mode fix)

I usually run these additional tests:

dartanalyzer --strong pkg/kernel/{lib,bin,test}
tools/test.py pkg/kernel -mrelease --checked
tools/test.py pkg/front_end -mrelease --checked
tools/test.py dart2js/kernel -mrelease --checked

I usually also check that the compiled dart2js binary is unchanged if I am not
expecting it to change.

In this case it actually has changed: the declaration order is different in some
cases.
It is generally more consistent with the original order now, so I'm happy to
keep it this way.
I made a small helper utility to help compare them:
https://gist.github.com/asgerf/adde37ed58fe984d53b82d362187c777

Perhaps mention in the CL description that this changes the declaration order,
in case this shows up in a 'git blame'.


I usually don't run the AOT tests offline as they are quite slow, but it's good
to know about them, since they can potentially break on the bots. This is how to
run them:

tools/build.py dart_precompiled_runtime
tools/test.py -cdartkp -rdart_precompiled language co19

They have had some issues with class finalization order so they could be
sensitive to this change, but I just ran the AOT language tests over lunch and
they were fine, so I wouldn't worry about it.

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:177: assert(false); // unreachable.
Was it intended to have both throw and assert(false)? I'd think the throw is
enough.

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:598: var units = [];
Add a type annotation here (to the list or the variable).

The dartanalyzer warns about this later:
Unsound implicit cast from List<dynamic> to List<CompilationUnit>

[Paul Berry, 2016-12-07 13:46:46]

lgtm assuming Asger's suggested fixes are made.

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:677: // TODO(jensj): Get this another way?
I'm guessing this TODO comment had to do with the call to computeNode on the
line below, so it should be deleted now.  +jensj, is that correct?

[jensj, 2016-12-07 14:02:21]

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:677: // TODO(jensj): Get this another way?
On 2016/12/07 13:46:46, Paul Berry wrote:
> I'm guessing this TODO comment had to do with the call to computeNode on the
> line below, so it should be deleted now.  +jensj, is that correct?

Actually it wasn't about the computeNode, but you are right, it is outdated and
should be removed.

[Siggi Cherem (dart-lang), 2016-12-07 15:18:08]

Description was changed from

==========
dartk: remove uses of .computeNode

Calling .computeNode() is sort of an anti-pattern for using analyzer. This was
however used very sparsely in dartk. The few places where it was used,
computeNode was practically called immediately after resolving the library
element, so I don't expect this to make almost any difference in performance.
==========

to

==========
dartk: remove uses of .computeNode

As a result, this changes the order in which declarations appear in the
generated output. The new order is more consistent with the textual order in the
original file.

Calling .computeNode() is sort of an anti-pattern for using analyzer. This was
however used very sparsely in dartk. The few places where it was used,
computeNode was practically called immediately after resolving the library
element, so I don't expect this to make almost any difference in performance.
==========

[Siggi Cherem (dart-lang), 2016-12-07 15:34:30]

Thanks for all the comments and for all the details on testing!

I may forget all the test combinations, so I copied them to a new wiki page
here:

  https://github.com/dart-lang/sdk/wiki/Kernel-developer-notes

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:177: assert(false); // unreachable.
On 2016/12/07 11:34:00, asgerf wrote:
> Was it intended to have both throw and assert(false)? I'd think the throw is
> enough.

Done. Oops, I started with the assert, but then added the throw to debug a case
I missed and forgot to remove the assert.

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:598: var units = [];
On 2016/12/07 11:34:00, asgerf wrote:
> Add a type annotation here (to the list or the variable).
> 
> The dartanalyzer warns about this later:
> Unsound implicit cast from List<dynamic> to List<CompilationUnit>

Done, thanks. I keep forgetting that I need to run dartanalyzer within the
project to get the appropriate strong-mode warnings.

Paul filed this issue, so we get the appropriate coverage when running test.py
-c dart2analyzer: https://github.com/dart-lang/sdk/issues/28013

https://codereview.chromium.org/2553303002/diff/40001/pkg/kernel/lib/analyzer...
pkg/kernel/lib/analyzer/loader.dart:677: // TODO(jensj): Get this another way?
On 2016/12/07 14:02:21, jensj wrote:
> On 2016/12/07 13:46:46, Paul Berry wrote:
> > I'm guessing this TODO comment had to do with the call to computeNode on the
> > line below, so it should be deleted now.  +jensj, is that correct?
> 
> Actually it wasn't about the computeNode, but you are right, it is outdated
and
> should be removed.

Done.

[Siggi Cherem (dart-lang), 2016-12-07 15:42:36]

Description was changed from

==========
dartk: remove uses of .computeNode

As a result, this changes the order in which declarations appear in the
generated output. The new order is more consistent with the textual order in the
original file.

Calling .computeNode() is sort of an anti-pattern for using analyzer. This was
however used very sparsely in dartk. The few places where it was used,
computeNode was practically called immediately after resolving the library
element, so I don't expect this to make almost any difference in performance.
==========

to

==========
dartk: remove uses of .computeNode

As a result, this changes the order in which declarations appear in the
generated output. The new order is more consistent with the textual order in the
original file.

Calling .computeNode() is sort of an anti-pattern for using analyzer. This was
however used very sparsely in dartk. The few places where it was used,
computeNode was practically called immediately after resolving the library
element, so I don't expect this to make almost any difference in performance.

R=asgerf@google.com, paulberry@google.com

Committed:
https://github.com/dart-lang/kernel/commit/f315ae1c00ee804167483d6a41be2bfe3d...
==========

[Siggi Cherem (dart-lang), 2016-12-07 15:42:37]

Committed patchset #2 (id:60001) manually as
f315ae1c00ee804167483d6a41be2bfe3d48fdc6 (presubmit successful).