Replace intptr_t with TokenDescriptor

Replace intptr_t with TokenDescriptor

- Use TokenDescriptor instead of intptr_t for all token positions.
- Use TokenDescriptor in raw_object instead of int32_t.
- TokenDescriptor is a POD with an int32_t (this shrinks the size of AST and IR nodes by 32-bits on 64-bit architectures).

There are some cleanups I plan on doing as a follow up CL:

- Replace TokenDescriptor::value() with TokenDescriptor::TokenPos()

R=iposva@google.com, rmacnak@google.com

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

[Cutch, 2016-01-27 23:54:17]

Description was changed from

==========
Add TokenDescriptor

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

to

==========
Replace intptr_t with TokenDescriptor

- Use TokenDescriptor instead of intptr_t for all token positions.
- Use TokenDescriptor in raw_object instead of int32_t.
- TokenDescriptor is a POD with an int32_t (this shrinks the size of AST and IR
nodes by 32-bits on 64-bit architectures).

There are some cleanups I plan on doing as a follow up CL:

- Replace TokenDescriptor::value() with TokenDescriptor::TokenPos()
==========

[Cutch, 2016-01-27 23:54:17]

johnmccutchan@google.com changed reviewers:
+ iposva@google.com, rmacnak@google.com

[Cutch, 2016-01-27 23:55:25]

All architectures are passing tests.

PTAL because I'd like to land this ASAP to avoid merge conflicts.

[rmacnak, 2016-01-28 18:21:50]

lgtm

https://chromiumcodereview.appspot.com/1644793002/diff/1/runtime/vm/token_des...
File runtime/vm/token_descriptor.h (right):

https://chromiumcodereview.appspot.com/1644793002/diff/1/runtime/vm/token_des...
runtime/vm/token_descriptor.h:32: // into the snapshot as an int32_t.
encoded into the snapshot and stored on the heap

https://chromiumcodereview.appspot.com/1644793002/diff/1/runtime/vm/token_des...
runtime/vm/token_descriptor.h:112: // Is |this| a classifying sentinel source
position?
+ Classifying positions are used by the profiler to group instructions whose
cost isn't naturally attributable to a source location.

https://chromiumcodereview.appspot.com/1644793002/diff/1/runtime/vm/token_des...
runtime/vm/token_descriptor.h:122: // Is |this| a synthetic source position?
+ Synthetic source positions are used by the profiler to attribute ticks to a
pieces of source, but ignored by the debugger as potential breakpoints.

[hausner, 2016-01-28 18:43:24]

hausner@google.com changed reviewers:
+ hausner@google.com

[hausner, 2016-01-28 18:43:25]

DBC:

I have a hard time digesting this change. I thought you were making a new type
that contains bitfields for the synthetic bit, and for the classifying
positions, and leaves the token position as a (slightly smaller) number.

With this change, you essentially introduce a wrapper type around an int32_t
named TokenDescriptor, but all the variables are still named xxx_pos. I don't
suggest to rename the variables to xxx_descriptor. That would be less readable.
I'd hope to see a change where the token positions remain integers wherever
possible.

I'd also suggest using less preprocessor trickery so that the IDE can find the
definition of constants.

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

https://codereview.chromium.org/1644793002/diff/1/runtime/vm/token_descriptor...
runtime/vm/token_descriptor.h:31: // NOTE: While token positions are passed
around as an intptr_t they are encoded
The intptr_t reference is not true anymore.

[hausner, 2016-01-28 19:07:36]

BTW, there is already a type called TokenDescriptor, in the Scanner class. What
you are trying to encode is more a position descriptor than a token descriptor.
Maybe call the type TokenPos? (although would have to rename the parser function
of the same name).


TokenPos::kNone  constant representing no source position

Convenient static functions to create new values:
TokenPos(0) for the first position in a script
TokenPos::ClassXYZ() for classifying position XYZ
TokenPos::Synthetic(123)  for a non-debuggable source position.

[Cutch, 2016-01-28 23:41:37]

On 2016/01/28 19:07:36, hausner wrote:
> BTW, there is already a type called TokenDescriptor, in the Scanner class.
What
> you are trying to encode is more a position descriptor than a token
descriptor.
> Maybe call the type TokenPos? (although would have to rename the parser
function
> of the same name).
> 
> 
> TokenPos::kNone  constant representing no source position
> 
> Convenient static functions to create new values:
> TokenPos(0) for the first position in a script
> TokenPos::ClassXYZ() for classifying position XYZ
> TokenPos::Synthetic(123)  for a non-debuggable source position.

I like the idea of calling it a TokenPos or some other name.

[Ivan Posva, 2016-02-01 17:16:53]

Some comments inline.

To answer the larger question: I suggest using TokenPosition as the name. As far
as I understand this should not collide with any other names and is shorter than
TokenDescriptor.

Overall this LGTM by making the position of a token in the code more abstract.

Thanks,
-Ivan

https://codereview.chromium.org/1644793002/diff/20001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:2022: token_pos =
TokenDescriptor(tkit.CurrentPosition());
Should we consider having the TokenStream::Iterator::CurrentPosition() return a
TokenDescriptor?

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast.cc#newco...
runtime/vm/ast.cc:105: token_pos().value(), vars_.length());
value() or ToCString()?

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_printer.cc
File runtime/vm/ast_printer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_printer....
runtime/vm/ast_printer.cc:34: scope->begin_token_pos().value(),
ditto.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:17: #define ST(token_pos)
TokenDescriptor::ToSynthetic(token_pos)
Why is ToSynthetic() not a method on TokenPosition?

#define ST(token_pos) ((token_pos).ToSynthetic())

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:2078: mixin_app_class.token_pos().value());
ditto.

Here and other places.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:563: new(Z)
StrictCompareInstr(TokenDescriptor(call_block->start_pos()),
Why is start_pos() not returning a TokenPosition?

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2293:
ASSERT(TokenDescriptor(node->token_pos()).IsSynthetic() ||
ditto.

Why not TokenPosition in the first place?

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/token_descri...
File runtime/vm/token_descriptor.h (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/token_descri...
runtime/vm/token_descriptor.h:66: inline bool operator<(const TokenDescriptor&
b) const {
These operators mean that synthesized locations are not considered when checking
for ranges. I assume that is intentional.

[Cutch, 2016-02-01 22:47:44]

Patch Set 3 uses TokenPosition instead of TokenDescriptor. Now addressing
individual comments.

[Cutch, 2016-02-02 18:00:20]

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

https://codereview.chromium.org/1644793002/diff/1/runtime/vm/token_descriptor...
runtime/vm/token_descriptor.h:31: // NOTE: While token positions are passed
around as an intptr_t they are encoded
On 2016/01/28 18:43:25, hausner wrote:
> The intptr_t reference is not true anymore.

Done.

https://codereview.chromium.org/1644793002/diff/1/runtime/vm/token_descriptor...
runtime/vm/token_descriptor.h:32: // into the snapshot as an int32_t.
On 2016/01/28 18:21:50, rmacnak wrote:
> encoded into the snapshot and stored on the heap

Acknowledged.

https://codereview.chromium.org/1644793002/diff/20001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/lib/mirrors.cc#...
runtime/lib/mirrors.cc:2022: token_pos =
TokenDescriptor(tkit.CurrentPosition());
On 2016/02/01 17:16:52, Ivan Posva wrote:
> Should we consider having the TokenStream::Iterator::CurrentPosition() return
a
> TokenDescriptor?

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast.cc
File runtime/vm/ast.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast.cc#newco...
runtime/vm/ast.cc:105: token_pos().value(), vars_.length());
On 2016/02/01 17:16:52, Ivan Posva wrote:
> value() or ToCString()?

ToCString(). Done here and elsewhere.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_printer.cc
File runtime/vm/ast_printer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_printer....
runtime/vm/ast_printer.cc:34: scope->begin_token_pos().value(),
On 2016/02/01 17:16:52, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_transfor...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/ast_transfor...
runtime/vm/ast_transformer.cc:17: #define ST(token_pos)
TokenDescriptor::ToSynthetic(token_pos)
On 2016/02/01 17:16:52, Ivan Posva wrote:
> Why is ToSynthetic() not a method on TokenPosition?
> 
> #define ST(token_pos) ((token_pos).ToSynthetic())

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:2078: mixin_app_class.token_pos().value());
On 2016/02/01 17:16:52, Ivan Posva wrote:
> ditto.
> 
> Here and other places.

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:2078: mixin_app_class.token_pos().value());
On 2016/02/01 17:16:52, Ivan Posva wrote:
> ditto.
> 
> Here and other places.

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:563: new(Z)
StrictCompareInstr(TokenDescriptor(call_block->start_pos()),
On 2016/02/01 17:16:52, Ivan Posva wrote:
> Why is start_pos() not returning a TokenPosition?

start_pos is actually an SSA position and not a token position.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2293:
ASSERT(TokenDescriptor(node->token_pos()).IsSynthetic() ||
On 2016/02/01 17:16:52, Ivan Posva wrote:
> ditto.
> 
> Why not TokenPosition in the first place?

Done.

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/token_descri...
File runtime/vm/token_descriptor.h (right):

https://codereview.chromium.org/1644793002/diff/20001/runtime/vm/token_descri...
runtime/vm/token_descriptor.h:66: inline bool operator<(const TokenDescriptor&
b) const {
On 2016/02/01 17:16:52, Ivan Posva wrote:
> These operators mean that synthesized locations are not considered when
checking
> for ranges. I assume that is intentional.

Acknowledged.

[Cutch, 2016-02-02 18:15:49]

Description was changed from

==========
Replace intptr_t with TokenDescriptor

- Use TokenDescriptor instead of intptr_t for all token positions.
- Use TokenDescriptor in raw_object instead of int32_t.
- TokenDescriptor is a POD with an int32_t (this shrinks the size of AST and IR
nodes by 32-bits on 64-bit architectures).

There are some cleanups I plan on doing as a follow up CL:

- Replace TokenDescriptor::value() with TokenDescriptor::TokenPos()
==========

to

==========
Replace intptr_t with TokenDescriptor

- Use TokenDescriptor instead of intptr_t for all token positions.
- Use TokenDescriptor in raw_object instead of int32_t.
- TokenDescriptor is a POD with an int32_t (this shrinks the size of AST and IR
nodes by 32-bits on 64-bit architectures).

There are some cleanups I plan on doing as a follow up CL:

- Replace TokenDescriptor::value() with TokenDescriptor::TokenPos()

R=iposva@google.com, rmacnak@google.com

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

[Cutch, 2016-02-02 18:15:50]

Committed patchset #6 (id:100001) manually as
d77d37612458b34773926b38b35ae58af34bebe8 (presubmit successful).