[Interpreter] Add ValidBytecodeOperandSizeCombinationsIterator.

[Interpreter] Add ValidBytecodeOperandSizeCombinationsIterator.

A ValidBytecodeOperandSizeCombinationsIterator iterates through
all valid combinations of OperandSize and Bytecode, saving two
nested loops and a check.

BUG=v8:4280
LOG=N

[oth, 2016-03-24 15:38:35]

oth@chromium.org changed reviewers:
+ oth@chromium.org

[oth, 2016-03-24 15:38:36]

Nice! A few minor comments, but looks like the right tool for the job.

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:550: bool HasNext() { return !reached_end_; }
HasNext() is fine and exists in the code base, but done() is more prominent and
used in the bytecode array iterator.

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:557: if (bytecode_ == Bytecode::kLast) {
Google C++ coding style would rather the larger code here goes into the
implementation file. I'd suggest moving all non-accessor code into bytecodes.cc
as bytecodes.h is a common include and inline implementations aren't needed
here.

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:577: static const Bytecode kFirstBytecode =
static_cast<Bytecode>(0);
Just to stop others writing troublesome code, consider adding
DISALLOW_COPY_AND_ASSIGN here.

[oth, 2016-03-24 15:53:41]

On 2016/03/24 15:38:36, oth wrote:
> Nice! A few minor comments, but looks like the right tool for the job.
> 
> https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h
> File src/interpreter/bytecodes.h (right):
> 
>
https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
> src/interpreter/bytecodes.h:550: bool HasNext() { return !reached_end_; }
> HasNext() is fine and exists in the code base, but done() is more prominent
and
> used in the bytecode array iterator.
> 
>
https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
> src/interpreter/bytecodes.h:557: if (bytecode_ == Bytecode::kLast) {
> Google C++ coding style would rather the larger code here goes into the
> implementation file. I'd suggest moving all non-accessor code into
bytecodes.cc
> as bytecodes.h is a common include and inline implementations aren't needed
> here.
> 
>
https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
> src/interpreter/bytecodes.h:577: static const Bytecode kFirstBytecode =
> static_cast<Bytecode>(0);
> Just to stop others writing troublesome code, consider adding
> DISALLOW_COPY_AND_ASSIGN here.

Let's look around for opportunities to use this CL as we won't be thanked for
committing code
that's not used. If we find those places, then we'll need a test too. Probably
just counting
the number of Advance steps is expected and the counts on how many times each
operand is reported.

[Stefano Sanfilippo, 2016-03-24 16:02:27]

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:550: bool HasNext() { return !reached_end_; }
On 2016/03/24 15:38:36, oth wrote:
> HasNext() is fine and exists in the code base, but done() is more prominent
and
> used in the bytecode array iterator.

Done.

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:557: if (bytecode_ == Bytecode::kLast) {
On 2016/03/24 15:38:36, oth wrote:
> Google C++ coding style would rather the larger code here goes into the
> implementation file. I'd suggest moving all non-accessor code into
bytecodes.cc
> as bytecodes.h is a common include and inline implementations aren't needed
> here.

Done.

https://codereview.chromium.org/1822043005/diff/1/src/interpreter/bytecodes.h...
src/interpreter/bytecodes.h:577: static const Bytecode kFirstBytecode =
static_cast<Bytecode>(0);
On 2016/03/24 15:38:36, oth wrote:
> Just to stop others writing troublesome code, consider adding
> DISALLOW_COPY_AND_ASSIGN here.

Done.

[rmcilroy, 2016-03-24 16:07:19]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-03-24 16:07:19]

https://codereview.chromium.org/1822043005/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1822043005/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:531: class BytecodesIterator final {
If we do ever land this I don't think we should call it BytecodesIterator,
that's far to easy to confuse with BytecodeArrayIterator. How about
ValidBytecodeIterator or similar.

[Stefano Sanfilippo, 2016-03-24 16:08:34]

Description was changed from

==========
[Interpreter] Add BytecodesIterator.

A BytecodesIterator iterates through all the valid combinations of
OperandSize and Bytecode, saving two nested loops and a check.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add BytecodesIterator.

A BytecodesIterator iterates through all valid combinations of
OperandSize and Bytecode, saving two nested loops and a check.

BUG=v8:4280
LOG=N
==========

[Stefano Sanfilippo, 2016-03-24 16:16:24]

Description was changed from

==========
[Interpreter] Add BytecodesIterator.

A BytecodesIterator iterates through all valid combinations of
OperandSize and Bytecode, saving two nested loops and a check.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Add ValidBytecodeOperandSizeCombinationsIterator.

A ValidBytecodeOperandSizeCombinationsIterator iterates through
all valid combinations of OperandSize and Bytecode, saving two
nested loops and a check.

BUG=v8:4280
LOG=N
==========

[Stefano Sanfilippo, 2016-03-24 16:16:34]

https://codereview.chromium.org/1822043005/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/1822043005/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:531: class BytecodesIterator final {
On 2016/03/24 16:07:19, rmcilroy wrote:
> If we do ever land this I don't think we should call it BytecodesIterator,
> that's far to easy to confuse with BytecodeArrayIterator. How about
> ValidBytecodeIterator or similar.

Done.

[Stefano Sanfilippo, 2016-03-24 16:58:11]

The CQ bit was checked by ssanfilippo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-24 16:58:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822043005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822043005/60001

[commit-bot: I haz the power, 2016-03-24 17:19:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-24 17:19:08]

Dry run: This issue passed the CQ dry run.