I missed the boat a bit a bit (as the code has landed!)... but I had a
number of
comments for next time someone touches the code!
https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/bytecode-array-builder.cc
File src/interpreter/bytecode-array-builder.cc (right):
https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/bytecode-array-builder.cc#newcode147
src/interpreter/bytecode-array-builder.cc:147:
DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 3);
This function does a DCHECK_EQ, the following functions use DCHECK, is
this intentional?
https://codereview.chromium.org/1266713004/diff/120001/src/interpreter/bytecode-array-builder.cc#newcode196
src/interpreter/bytecode-array-builder.cc:196: return
static_cast<Bytecode>(-1);
Should we have an error ByteCode that is -1, instead of casting it here?
https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode11614
src/objects.cc:11614: int bytes = 0;
Can we name 'bytes' to say what this is (the size of a bytecode+params,
the size of a function?)? It is not clear!
https://codereview.chromium.org/1266713004/diff/120001/src/objects.cc#newcode11628
src/objects.cc:11628: for (int j = 1; j < bytes; j++) {
Should this for-loop be pulled out into a separate function? It looks to
be conceptually at a lower level (disassembling a single instruction?).
Also, the value of 'j' is used as j-1, j and j+1. Is there a way to
rework this to not need some many variations!
https://codereview.chromium.org/1266713004/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
