Lgtm. Comments are mostly nitpicking.
http://codereview.chromium.org/11228/diff/1/9 File src/bytecodes-re2k.h (right): http://codereview.chromium.org/11228/diff/1/9#newcode67 Line 67: #ifdef DEBUG Does this have to be #ifdef DEBUG? Unused static consts have no effect anyway right? http://codereview.chromium.org/11228/diff/1/13 File src/interpreter-re2k.cc (right): http://codereview.chromium.org/11228/diff/1/13#newcode42 Line 42: # define BYTECODE(name) break; \ I would prefer if the breaks were written explicitly everywhere. This is too subtle for my taste. But feel free to disagree and not change it. http://codereview.chromium.org/11228/diff/1/13#newcode44 Line 44: if (FLAG_trace_regexp_bytecodes) { \ Couldn't this be factored into a function rather than duplicated? http://codereview.chromium.org/11228/diff/1/13#newcode169 Line 169: pc += 7; I would suggest using constants for opcode length so the literal value occurs exactly one place. I've previously used the approach of packaging all the information about a bytecode set into constants on a class so you could do OpcodeInfo<CHECK_GT>::kLength, OpcodeInfo<CHECH_GT>::kArgCount, etc., so you would always just use pc += OpcodeInfo<[current opcode]>::kLength. That may be too spacy for our use though. http://codereview.chromium.org/11228/diff/1/19 File src/regexp-macro-assembler-re2k.h (right): http://codereview.chromium.org/11228/diff/1/19#newcode57 Line 57: uc16 limit, These should fit on one line. http://codereview.chromium.org/11228/diff/1/4 File test/mjsunit/unicode-test.js (right): http://codereview.chromium.org/11228/diff/1/4#newcode9132 Line 9132: //dump_re(re); ? http://codereview.chromium.org/11228 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
