[kernel] Offsets on loops

[kernel] Offsets on loops

BUG=
R=ahe@google.com, johnniwinther@google.com

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

[jensj, 2017-08-08 10:47:36]

jensj@google.com changed reviewers:
+ ahe@google.com, johnniwinther@google.com

[jensj, 2017-08-08 10:47:37]

Johnni requested offsets on loops. Here we go.

[Johnni Winther, 2017-08-08 11:00:42]

lgtm

https://codereview.chromium.org/2999633002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2999633002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3437: /// Valid values are from 0 and up, or -1
([TreeNode.noOffset]) if the file offset is
Long line.

[ahe, 2017-08-08 11:38:51]

LGTM, but I have some general remarks about comments :-)

Also, there's an important question about why we need to store bodyOffset
instead of using body.fileOffset?

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3439: int bodyOffset = TreeNode.noOffset;
Why is the bodyOffset different from body.fileOffset?

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_to_binary.dart:977: writeOffset(node.bodyOffset);
Notice how we don't need to document that we're writing the offset of the body.
This information is implicit in the names of the variables. Contrast this to the
comments in the flow graph builder.

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary_flowgraph.cc (right):

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:4111: ReadPosition();    // read position.
print("Hello, World!"); // Prints "Hello, World!"

That's how you add comments to an example used in introducing students to
programming.

Repeating exactly the name of the method isn't a useful comment and should be
avoided.

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:4112: SkipExpression();  // read
condition.
Similarly, adding a comment that contradicts with the code isn't useful, and
should be removed.

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:4123: Tag tag = ReadTag();              
// Read first part of condition.
Generally, comments should start with an uppercase letter as here. And contain
information that isn't obvious from the code.

However, this code could be made self explanatory:

Tag conditionStart = ReadTag();

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:4133: ReadPosition();             // read
position.
Read position of what?

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:4134: ReadPosition();             // read
body position.
This comment adds value as it adds additional information (that it's the
position of the *body*).

https://codereview.chromium.org/2999633002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary_flowgraph.cc:6223: ReadPosition();                     
          // read position.
Of what?

[jensj, 2017-08-08 11:54:53]

Peter: In regards to all comments in kernel_binary_flowgraph.cc I do realize
that "ReadPosition(); // read position." does seem stupid.

It is done, though, in an attempt to make it explicit where we read from the
binary vs where we do not as the reading of the binary and the creation of the
flowgraph is mixed.

It should allow you to easily match up the reading of kernel_binary_flowgraph.cc
and the reading of binary.md.

For instance the comment that you say contradicts the code (while):
```
ReadPosition();    // read position.
SkipExpression();  // read condition.
SkipStatement();   // read body.
```

fits nicely with the binary.md
```
type WhileStatement extends Statement {
  Byte tag = 67;
  FileOffset fileOffset;
  Expression condition;
  Statement body;
}
```

so while we might "skip" rather than "read", we do in fact read from the binary,
thus advancing what we will read next time.

You might still argue that it is obvious that "skipping" skips something (in
this case in the binary), but I for one prefer the comment-style as - to me - it
is easier to scan through the file, looking for "// read {whatever}" vs scanning
through the file looking for either "SkipWhatever" or "ReadWhatever" or
"BuildWhatever" etc.

I'm not saying that this is perfect, I'm saying that in the current state of
affairs I prefer the code with these comments than without them. Things might
look different once we change the format.

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3439: int bodyOffset = TreeNode.noOffset;
On 2017/08/08 11:38:50, ahe wrote:
> Why is the bodyOffset different from body.fileOffset?

As I recall I had to add that because we don't have offsets on everything. So
body.fileOffset might not go into the dill file, but we need the position to
reproduce the debugging behavior of the non-kernel pipeline.

[ahe, 2017-08-08 11:58:03]

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:3439: int bodyOffset = TreeNode.noOffset;
On 2017/08/08 11:54:53, jensj wrote:
> On 2017/08/08 11:38:50, ahe wrote:
> > Why is the bodyOffset different from body.fileOffset?
> 
> As I recall I had to add that because we don't have offsets on everything. So
> body.fileOffset might not go into the dill file, but we need the position to
> reproduce the debugging behavior of the non-kernel pipeline.

Can't we make sure that we have positions on all those other things? It seems
like a bug if we have statements that lack a position, or am I missing
something?

[ahe, 2017-08-08 11:59:57]

On 2017/08/08 11:54:53, jensj wrote:
> Peter: In regards to all comments in kernel_binary_flowgraph.cc I do realize
> that "ReadPosition(); // read position." does seem stupid.
> 
> It is done, though, in an attempt to make it explicit where we read from the
> binary vs where we do not as the reading of the binary and the creation of the
> flowgraph is mixed.
> 
> It should allow you to easily match up the reading of
kernel_binary_flowgraph.cc
> and the reading of binary.md.
> 
> For instance the comment that you say contradicts the code (while):
> ```
> ReadPosition();    // read position.
> SkipExpression();  // read condition.
> SkipStatement();   // read body.
> ```
> 
> fits nicely with the binary.md
> ```
> type WhileStatement extends Statement {
>   Byte tag = 67;
>   FileOffset fileOffset;
>   Expression condition;
>   Statement body;
> }
> ```
> 
> so while we might "skip" rather than "read", we do in fact read from the
binary,
> thus advancing what we will read next time.
> 
> You might still argue that it is obvious that "skipping" skips something (in
> this case in the binary), but I for one prefer the comment-style as - to me -
it
> is easier to scan through the file, looking for "// read {whatever}" vs
scanning
> through the file looking for either "SkipWhatever" or "ReadWhatever" or
> "BuildWhatever" etc.
> 
> I'm not saying that this is perfect, I'm saying that in the current state of
> affairs I prefer the code with these comments than without them. Things might
> look different once we change the format.

Personally, I find scanning comments to be unreliable, but please notice that
I'm just expressing an opinion, and it is perfectly fine for you to disagree
with my point of view :-)

[jensj, 2017-08-08 12:26:28]

On 2017/08/08 11:58:03, ahe wrote:
> https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart
> File pkg/kernel/lib/ast.dart (right):
> 
>
https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart...
> pkg/kernel/lib/ast.dart:3439: int bodyOffset = TreeNode.noOffset;
> On 2017/08/08 11:54:53, jensj wrote:
> > On 2017/08/08 11:38:50, ahe wrote:
> > > Why is the bodyOffset different from body.fileOffset?
> > 
> > As I recall I had to add that because we don't have offsets on everything.
So
> > body.fileOffset might not go into the dill file, but we need the position to
> > reproduce the debugging behavior of the non-kernel pipeline.
> 
> Can't we make sure that we have positions on all those other things? It seems
> like a bug if we have statements that lack a position, or am I missing
> something?

I don't know what the correct thing is.
I think that most - if not all - positions we currently have (in the dill file)
was some I added as needed to make debugging tests (service tests) work.

[ahe, 2017-08-08 12:28:12]

On 2017/08/08 12:26:28, jensj wrote:
> On 2017/08/08 11:58:03, ahe wrote:
> >
https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart
> > File pkg/kernel/lib/ast.dart (right):
> > 
> >
>
https://codereview.chromium.org/2999633002/diff/40001/pkg/kernel/lib/ast.dart...
> > pkg/kernel/lib/ast.dart:3439: int bodyOffset = TreeNode.noOffset;
> > On 2017/08/08 11:54:53, jensj wrote:
> > > On 2017/08/08 11:38:50, ahe wrote:
> > > > Why is the bodyOffset different from body.fileOffset?
> > > 
> > > As I recall I had to add that because we don't have offsets on everything.
> So
> > > body.fileOffset might not go into the dill file, but we need the position
to
> > > reproduce the debugging behavior of the non-kernel pipeline.
> > 
> > Can't we make sure that we have positions on all those other things? It
seems
> > like a bug if we have statements that lack a position, or am I missing
> > something?
> 
> I don't know what the correct thing is.
> I think that most - if not all - positions we currently have (in the dill
file)
> was some I added as needed to make debugging tests (service tests) work.

Perhaps we should figure out what the correct thing is?

[jensj, 2017-08-09 06:39:32]

Description was changed from

==========
[kernel] Offsets on loops

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

to

==========
[kernel] Offsets on loops

BUG=
R=ahe@google.com, johnniwinther@google.com

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

[jensj, 2017-08-09 06:39:33]

Committed patchset #3 (id:40001) manually as
f6d6898bce85ffcf72788b4dafae31f2e97ad86d (presubmit successful).