One more iteration of PcDescriptor iterator improvement: do not copy record but access individual …

ONe more iteration of PcDesacriptor iterator imporvement: do not copy record but access individual values when needed.

R=hausner@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38408

[hausner, 2014-07-17 23:22:56]

LGTM with one suggestion (see below). I would separate the advancing of the
iterator from the access to its fields. the you can get rid of duplicating all
accessors and make client code less confusing.

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

https://codereview.chromium.org/403643002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:10361: while (iter.HasNext()) {
See other comment. This loop would become:

while (iter.HasNext()) {
  iter.MoveNext();
  uword pc = iter.pc();
  len += OS::SNPrint(NULL, 0, kFormat, addr_width,
              pc,
              KindAsStr(iter.kind()),
              iter.deopt_id(),
              iter.token_pos(),
              iter.try_index());
}

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

https://codereview.chromium.org/403643002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:3059: intptr_t NextDeoptId() {
I find it a bit awkward that you have two accessors for each value, say
current_deopt_id() and NextDeoptId(), the latter having a side effect of moving
the iterator.

How about this iterator interface:
HasNext() // true if there is another entry
MoveNext() // moves the iterator
DeoptId() // get deopt id of current descriptor
TokenPos() // get token position of current descriptor 
etc...

Iterating over a pc descriptor array would then be:

PcDescriptors::Iterator i(desc, mask);
while (i.HasNext()) {
  i.MoveNext();
  // access all fields that you need
}

[srdjan, 2014-07-18 18:16:45]

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

https://codereview.chromium.org/403643002/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:10361: while (iter.HasNext()) {
On 2014/07/17 23:22:56, hausner wrote:
> See other comment. This loop would become:
> 
> while (iter.HasNext()) {
>   iter.MoveNext();
>   uword pc = iter.pc();
>   len += OS::SNPrint(NULL, 0, kFormat, addr_width,
>               pc,
>               KindAsStr(iter.kind()),
>               iter.deopt_id(),
>               iter.token_pos(),
>               iter.try_index());
> }

Good suggestion, changed to:

while (iter.MoveNext()) {
  ...iter.Pc(), iter.Kind()...
}

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

https://codereview.chromium.org/403643002/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:3059: intptr_t NextDeoptId() {
On 2014/07/17 23:22:56, hausner wrote:
> I find it a bit awkward that you have two accessors for each value, say
> current_deopt_id() and NextDeoptId(), the latter having a side effect of
moving
> the iterator.
> 
> How about this iterator interface:
> HasNext() // true if there is another entry
> MoveNext() // moves the iterator
> DeoptId() // get deopt id of current descriptor
> TokenPos() // get token position of current descriptor 
> etc...
> 
> Iterating over a pc descriptor array would then be:
> 
> PcDescriptors::Iterator i(desc, mask);
> while (i.HasNext()) {
>   i.MoveNext();
>   // access all fields that you need
> }


Good suggestion, changed to:

while (i.MoveNext()) {
   // access all fields that you need
}

[hausner, 2014-07-18 18:36:57]

Even betterer!

[srdjan, 2014-07-18 20:11:37]

Committed patchset #3 manually as r38408 (presubmit successful).