Add documentationComment for Class to Kernel. Parse it. Resynthesize in Analyzer.

Add documentationComment for Class to Kernel. Parse it. Resynthesize in Analyzer.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Paul Berry, 2017-07-14 22:03:08]

lgtm, but I'd like to hear someone else's thoughts about
	runtime/vm/kernel_binary_flowgraph.h before we land this.

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.h:1268: documentation_comment_index_ =
builder_->ReadStringReference();
Would it be better for the VM to just skip documentation comments, rather than
store them in a field that is never referenced?  They don't affect semantics so
it seems silly to bloat the VM's internal code representation with them.

[Siggi Cherem (dart-lang), 2017-07-14 22:12:43]

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

[Siggi Cherem (dart-lang), 2017-07-14 22:12:44]

I believe Kevin and Jens are on vacation for a few weeks.

+asiva - not sure if you can take a look or know someone around that can?

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.h:1268: documentation_comment_index_ =
builder_->ReadStringReference();
On 2017/07/14 22:03:08, Paul Berry wrote:
> Would it be better for the VM to just skip documentation comments, rather than
> store them in a field that is never referenced?  They don't affect semantics
so
> it seems silly to bloat the VM's internal code representation with them.

I share your concerns too, I like the idea of just skipping the bits.

[scheglov, 2017-07-15 00:41:57]

PTAL, I fixed the native code.

It's not like the change to kernel_binary_flowgraph.h is anything more than just
mechanical replication of already existing similar code. I would be sad to wait
longer than several hours for reviews of such trivial changes. Much less days or
weeks. All front_end tests pass, as I understand, this means that we run it
successfully.

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
File runtime/vm/kernel_binary_flowgraph.h (right):

https://codereview.chromium.org/2977133002/diff/1/runtime/vm/kernel_binary_fl...
runtime/vm/kernel_binary_flowgraph.h:1268: documentation_comment_index_ =
builder_->ReadStringReference();
On 2017/07/14 22:12:43, Siggi Cherem (dart-lang) wrote:
> On 2017/07/14 22:03:08, Paul Berry wrote:
> > Would it be better for the VM to just skip documentation comments, rather
than
> > store them in a field that is never referenced?  They don't affect semantics
> so
> > it seems silly to bloat the VM's internal code representation with them.
> 
> I share your concerns too, I like the idea of just skipping the bits.

I agree, we can just skip it.

[Siggi Cherem (dart-lang), 2017-07-15 02:36:03]

I'm OK moving forward with this kind of changes in the .ht file and address
concerns as a follow up if there are any. So LGTM on that front.

(Note I didn't review in detail the changes to fe/analyzer since Paul already
did.)

In terms of tests, when making changes that may affect the vm, it is a good
sanity check to run:


./tools/build.py -m release
./tools/test.py -m release -c dartk -r vm

You can also run the aot-compiler tests, but these are less likely to fail if
the dark/vm tests pass:
./tools/test.py -m release -c dartkp -r dart_precompiled

without any suite arguments that includes a lot of tests (~12K), sometimes you
get enough coverage of the unit tests by running the `vm` and `standalone`
suites:

./tools/test.py -m release -c dartk -r vm vm standalone

[scheglov, 2017-07-15 16:25:19]

On 2017/07/15 02:36:03, Siggi Cherem (dart-lang) wrote:
> I'm OK moving forward with this kind of changes in the .ht file and address
> concerns as a follow up if there are any. So LGTM on that front.
> 
> (Note I didn't review in detail the changes to fe/analyzer since Paul already
> did.)
> 
> In terms of tests, when making changes that may affect the vm, it is a good
> sanity check to run:
> 
> 
> ./tools/build.py -m release
> ./tools/test.py -m release -c dartk -r vm
> 
> You can also run the aot-compiler tests, but these are less likely to fail if
> the dark/vm tests pass:
> ./tools/test.py -m release -c dartkp -r dart_precompiled
> 
> without any suite arguments that includes a lot of tests (~12K), sometimes you
> get enough coverage of the unit tests by running the `vm` and `standalone`
> suites:
> 
> ./tools/test.py -m release -c dartk -r vm vm standalone

OK

I run successfully all the "tools/test.py -m release -c dartk -r vm" tests
"[05:47 | 100% | +15855 | -    0]".

"tools/test.py -m release -c dartkp -r dart_precompiled" has some failures, but
the same number as without the change "[46:34 | 100% | +13950 | -  564]".

[scheglov, 2017-07-15 16:53:55]

Description was changed from

==========
Add documentationComment for Class to Kernel. Parse it. Resynthesize in
Analyzer.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Add documentationComment for Class to Kernel. Parse it. Resynthesize in
Analyzer.

R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-07-15 16:53:57]

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