[kernel] Don't use kernel ast nodes as keys

[kernel] Don't use kernel ast nodes as keys

ScopeBuildingResult now uses 'IntMap's instead, i.e. doesn't use kernel objects as key
function_scopes doesn't contain function but dill file offset

BUG=
R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/bc7200c392494a11f1df3f6b6df661d3fee27108
Committed: https://github.com/dart-lang/sdk/commit/00ef784f0c9e34ace03a68c3a6bc6cc03dba69ab

[jensj, 2017-03-27 09:10:59]

jensj@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-03-28 06:42:23]

I suggest simplifying it by putting an offset on every tree node and getting rid
of the need for templates.

The offset might as well be intptr_t, I don't know why we used int64_t.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel.h#newcode719
runtime/vm/kernel.h:719: int64_t kernelFileOffset_;
I wonder why this is int64_t.

Call the field kernel_file_offset_;

(Though I think 'file' is a noise word here anyway.  It's not necessarily a file
at all except in the sense that an array of bytes is a file.)

Please document it.  Specifically, I want to know that it is the offset of just
after the tag.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:62: void ScopeBuilder::EnterScope(NewScopeType* node,
Instead of the template functions I'd rather add offset to every Kernel
TreeNode, set it to a default of -1 or some other invalid value, and assert that
it's valid here.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2017: Fragment FlowGraphBuilder::EnterScope(int64_t
kernel_offset,
It's a little confusing that this is sometimes 'kernel_offset' and sometimes
'kernel_file_offset'.  I suggest something uniformly 'kernel_offset'.

Anyway, can't you just pass the node here and use node->kernel_offset() in this
function, instead of having to have ->kernel_offset() at every call site for
EnterScope and ExitScope?

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:553: int64_t kernelFileOffset;
This should be named kernel_file_offset to fit the coding style, but I think it
would be better as kernel_offset.

[jensj, 2017-03-28 08:56:29]

Thanks. PTAL.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel.h#newcode719
runtime/vm/kernel.h:719: int64_t kernelFileOffset_;
On 2017/03/28 06:42:22, Kevin Millikin (Google) wrote:
> I wonder why this is int64_t.
> 
> Call the field kernel_file_offset_;
> 
> (Though I think 'file' is a noise word here anyway.  It's not necessarily a
file
> at all except in the sense that an array of bytes is a file.)
> 
> Please document it.  Specifically, I want to know that it is the offset of
just
> after the tag.

Done.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc
File runtime/vm/kernel_to_il.cc (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:62: void ScopeBuilder::EnterScope(NewScopeType* node,
On 2017/03/28 06:42:23, Kevin Millikin (Google) wrote:
> Instead of the template functions I'd rather add offset to every Kernel
> TreeNode, set it to a default of -1 or some other invalid value, and assert
that
> it's valid here.

Done.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:2017: Fragment FlowGraphBuilder::EnterScope(int64_t
kernel_offset,
On 2017/03/28 06:42:22, Kevin Millikin (Google) wrote:
> It's a little confusing that this is sometimes 'kernel_offset' and sometimes
> 'kernel_file_offset'.  I suggest something uniformly 'kernel_offset'.
> 
> Anyway, can't you just pass the node here and use node->kernel_offset() in
this
> function, instead of having to have ->kernel_offset() at every call site for
> EnterScope and ExitScope?

Done.

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2778693002/diff/1/runtime/vm/kernel_to_il.h#n...
runtime/vm/kernel_to_il.h:553: int64_t kernelFileOffset;
On 2017/03/28 06:42:23, Kevin Millikin (Google) wrote:
> This should be named kernel_file_offset to fit the coding style, but I think
it
> would be better as kernel_offset.

Done.

[Kevin Millikin (Google), 2017-03-28 12:30:12]

LGTM.

https://codereview.chromium.org/2778693002/diff/20001/runtime/vm/kernel_to_il.h
File runtime/vm/kernel_to_il.h (right):

https://codereview.chromium.org/2778693002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:88: typedef int64_t Key;
intptr_t?

https://codereview.chromium.org/2778693002/diff/20001/runtime/vm/kernel_to_il...
runtime/vm/kernel_to_il.h:553: int64_t kernel_offset;
intptr_t?

[jensj, 2017-03-29 06:39:44]

Description was changed from

==========
[kernel] Don't use kernel ast nodes as keys

ScopeBuildingResult now uses 'IntMap's instead, i.e. doesn't use kernel objects
as key
function_scopes doesn't contain function but dill file offset

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

to

==========
[kernel] Don't use kernel ast nodes as keys

ScopeBuildingResult now uses 'IntMap's instead, i.e. doesn't use kernel objects
as key
function_scopes doesn't contain function but dill file offset

BUG=
R=kmillikin@google.com

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

[jensj, 2017-03-29 06:39:46]

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

[jensj, 2017-03-29 07:57:19]

Description was changed from

==========
[kernel] Don't use kernel ast nodes as keys

ScopeBuildingResult now uses 'IntMap's instead, i.e. doesn't use kernel objects
as key
function_scopes doesn't contain function but dill file offset

BUG=
R=kmillikin@google.com

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

to

==========
[kernel] Don't use kernel ast nodes as keys

ScopeBuildingResult now uses 'IntMap's instead, i.e. doesn't use kernel objects
as key
function_scopes doesn't contain function but dill file offset

BUG=
R=kmillikin@google.com

Committed:
https://github.com/dart-lang/sdk/commit/bc7200c392494a11f1df3f6b6df661d3fee27108
Committed:
https://github.com/dart-lang/sdk/commit/00ef784f0c9e34ace03a68c3a6bc6cc03dba69ab
==========

[jensj, 2017-03-29 07:57:21]

Committed patchset #4 (id:60001) manually as
00ef784f0c9e34ace03a68c3a6bc6cc03dba69ab (presubmit successful).