Make the parser agnostic to the TokenStream implementation. This is the first step towards compacti…

Make the parser agnostic to the TokenStream implementation. This is the first step towards compacting the token stream.

Pull the data stream writer/reader code out of snapshot into a generic file so that it can be reused into the token stream compaction implementation.
Committed: https://code.google.com/p/dart/source/detail?r=9043

[hausner, 2012-06-22 00:02:27]

I think the interface to the iterator should change, it's too confusing now.

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h
File vm/datastream.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h#newcode14
vm/datastream.h:14: static const int8_t kSerializedBitsPerByte = 7;
I know you just copied this code from another file, so leaving it unchanged is
fine. I would find it more readable though if the names were shorter and did not
have calculations in them. For example, the mask and end byte marker could be
fixed values just like the min and max values.

http://codereview.chromium.org/10632009/diff/5001/vm/parser.cc
File vm/parser.cc (left):

http://codereview.chromium.org/10632009/diff/5001/vm/parser.cc#oldcode330
vm/parser.cc:330: CompilerStats::num_tokens_total += parser.tokens_.Length();
The number of tokens will not be equivalent to the length of the token stream.

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h
File vm/parser.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h#newcode116
vm/parser.h:116: intptr_t Length() const { return tokens_.Length(); }
What is the "Length" of an iterator? Do you mean the index of the token at the
current token position? Or the number of tokens iterated over since the iterator
was initialized?

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h#newcode122
vm/parser.h:122: intptr_t CurrentIndex() const { return token_index_; }
I would not call it an index anymore. What you abstract is a position on the
stream, not a particular index. They happen to be the same currently, but will
be different once the tokens become variable length.

[siva, 2012-06-22 17:59:02]

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h
File vm/datastream.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h#newcode14
vm/datastream.h:14: static const int8_t kSerializedBitsPerByte = 7;
I have renamed the constants removing the Serialized part as that only applied
in the snapshot context.

but I disagree with your comment about not having calculations in them. When one
constant is dependent on another constant it makes sense to use them in that
fashion, otherwise one would have to add a comment explaining that the
subsequent constant is dependent etc. etc.

On 2012/06/22 00:02:27, hausner wrote:
> I know you just copied this code from another file, so leaving it unchanged is
> fine. I would find it more readable though if the names were shorter and did
not
> have calculations in them. For example, the mask and end byte marker could be
> fixed values just like the min and max values.

http://codereview.chromium.org/10632009/diff/5001/vm/parser.cc
File vm/parser.cc (left):

http://codereview.chromium.org/10632009/diff/5001/vm/parser.cc#oldcode330
vm/parser.cc:330: CompilerStats::num_tokens_total += parser.tokens_.Length();
Changed name Length() to NumberOfTokens() in the stream.

On 2012/06/22 00:02:27, hausner wrote:
> The number of tokens will not be equivalent to the length of the token stream.

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h
File vm/parser.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h#newcode116
vm/parser.h:116: intptr_t Length() const { return tokens_.Length(); }
Renamed Length() to NumberOfTokens() basically it is the number of tokens in
this stream.

On 2012/06/22 00:02:27, hausner wrote:
> What is the "Length" of an iterator? Do you mean the index of the token at the
> current token position? Or the number of tokens iterated over since the
iterator
> was initialized?

http://codereview.chromium.org/10632009/diff/5001/vm/parser.h#newcode122
vm/parser.h:122: intptr_t CurrentIndex() const { return token_index_; }
Renamed this to CurrentPosition

On 2012/06/22 00:02:27, hausner wrote:
> I would not call it an index anymore. What you abstract is a position on the
> stream, not a particular index. They happen to be the same currently, but will
> be different once the tokens become variable length.

[hausner, 2012-06-22 18:54:36]

LGTM.

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h
File vm/datastream.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h#newcode14
vm/datastream.h:14: static const int8_t kSerializedBitsPerByte = 7;
Thanks for making the names shorter. I still have a nit though: if you think
that the calculations are the right way, why do you not calculate 63 and -64 and
127? Feel free to ignore the comment though.
 
On 2012/06/22 17:59:02, asiva wrote:
> I have renamed the constants removing the Serialized part as that only applied
> in the snapshot context.
> 
> but I disagree with your comment about not having calculations in them. When
one
> constant is dependent on another constant it makes sense to use them in that
> fashion, otherwise one would have to add a comment explaining that the
> subsequent constant is dependent etc. etc.
> 
> On 2012/06/22 00:02:27, hausner wrote:
> > I know you just copied this code from another file, so leaving it unchanged
is
> > fine. I would find it more readable though if the names were shorter and did
> not
> > have calculations in them. For example, the mask and end byte marker could
be
> > fixed values just like the min and max values.
>

http://codereview.chromium.org/10632009/diff/8001/vm/parser.h
File vm/parser.h (right):

http://codereview.chromium.org/10632009/diff/8001/vm/parser.h#newcode125
vm/parser.h:125: void AdvanceToNextToken() { token_position_ += 1; }
I prefer shorter names, e.g. Advance() here. This method name lives in the
context of a TokenStreamIterator, so it's pretty clear what the method advances
to.

[siva, 2012-06-22 20:36:29]

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h
File vm/datastream.h (right):

http://codereview.chromium.org/10632009/diff/5001/vm/datastream.h#newcode14
vm/datastream.h:14: static const int8_t kSerializedBitsPerByte = 7;
Used calculations for 63, -64, 127 etc.

On 2012/06/22 18:54:37, hausner wrote:
> Thanks for making the names shorter. I still have a nit though: if you think
> that the calculations are the right way, why do you not calculate 63 and -64
and
> 127? Feel free to ignore the comment though.
>  
> On 2012/06/22 17:59:02, asiva wrote:
> > I have renamed the constants removing the Serialized part as that only
applied
> > in the snapshot context.
> > 
> > but I disagree with your comment about not having calculations in them. When
> one
> > constant is dependent on another constant it makes sense to use them in that
> > fashion, otherwise one would have to add a comment explaining that the
> > subsequent constant is dependent etc. etc.
> > 
> > On 2012/06/22 00:02:27, hausner wrote:
> > > I know you just copied this code from another file, so leaving it
unchanged
> is
> > > fine. I would find it more readable though if the names were shorter and
did
> > not
> > > have calculations in them. For example, the mask and end byte marker could
> be
> > > fixed values just like the min and max values.
> > 
>

http://codereview.chromium.org/10632009/diff/8001/vm/parser.h
File vm/parser.h (right):

http://codereview.chromium.org/10632009/diff/8001/vm/parser.h#newcode125
vm/parser.h:125: void AdvanceToNextToken() { token_position_ += 1; }
On 2012/06/22 18:54:37, hausner wrote:
> I prefer shorter names, e.g. Advance() here. This method name lives in the
> context of a TokenStreamIterator, so it's pretty clear what the method
advances
> to.

Done.