Merge regexp2000 back into bleeding_edge

Merge regexp2000 back into bleeding_edge


Committed: http://code.google.com/p/v8/source/detail?r=832

[Christian Plesner Hansen, 2008-11-25 10:28:32]

Here's the regexp2000 merge.  I've looked through it and it looks okay to me.

[Lasse Reichstein, 2008-11-25 10:34:45]

LGTM

http://codereview.chromium.org/12427/diff/1/46
File src/assembler-ia32.cc (right):

http://codereview.chromium.org/12427/diff/1/46#newcode431
Line 431: int pos = label->pos() + Code::kHeaderSize;
Known bug: Off-by-one. Must subtract kHeapObjectTag as well, so result becomes
relative to actual Code* value.
(I'll fix it post-merge)

http://codereview.chromium.org/12427/diff/1/46#newcode872
Line 872: void Assembler::rep_cmpsb() {
Should this be split into cld, rep and cmpsb? The combination seems to belong in
macro_assembler, not plain assembler.

http://codereview.chromium.org/12427/diff/1/46#newcode1340
Line 1340: long_at_put(fixup_pos, pos + Code::kHeaderSize);
Off-by-one here too.

[Erik Corry, 2008-11-25 11:03:52]

Does it lint?

LGTM

http://codereview.chromium.org/12427/diff/1/29
File src/SConscript (right):

http://codereview.chromium.org/12427/diff/1/29#newcode48
Line 48: 'scopeinfo.cc', 'scopes.cc', 'serialize.cc', 'snapshot-common.cc',
This line has a trailing space.

http://codereview.chromium.org/12427/diff/1/29#newcode58
Line 58: 'macro-assembler-ia32.cc', 'regexp-macro-assembler-ia32.cc',
This line has a trailing space.

http://codereview.chromium.org/12427/diff/1/34
File src/assembler-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/34#newcode1
Line 1: // Copyright 2006-2008 the V8 project authors. All rights reserved.
Missing license boilerplate.

http://codereview.chromium.org/12427/diff/1/18
File src/flag-definitions.h (right):

http://codereview.chromium.org/12427/diff/1/18#newcode294
Line 294: DEFINE_bool(trace_regexp_bytecodes, false, "trace Irregexp bytecode
executon")
speling eror

http://codereview.chromium.org/12427/diff/1/40
File src/jsregexp-inl.h (right):

http://codereview.chromium.org/12427/diff/1/40#newcode1
Line 1: // Copyright 2006-2008 the V8 project authors. All rights reserved.
Correct year?

http://codereview.chromium.org/12427/diff/1/48
File src/jsregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/48#newcode28
Line 28: #define _HAS_EXCEPTIONS 0
??

http://codereview.chromium.org/12427/diff/1/48#newcode29
Line 29: #include <set>
Where is this coming from?

http://codereview.chromium.org/12427/diff/1/48#newcode840
Line 840: // New regular expression engine
Probably should mention "Irregexp"

http://codereview.chromium.org/12427/diff/1/48#newcode1228
Line 1228: Label *char_is_in_class =
Misplaced asterisk.

http://codereview.chromium.org/12427/diff/1/26
File src/jsregexp.h (right):

http://codereview.chromium.org/12427/diff/1/26#newcode696
Line 696: Guard *guard,
Misplaced asterisk.

http://codereview.chromium.org/12427/diff/1/26#newcode741
Line 741: DispatchTable *table_;
Misplaced asterisk.

http://codereview.chromium.org/12427/diff/1/3
File test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/12427/diff/1/3#newcode1
Line 1: // Copyright 2006-2008 the V8 project authors. All rights reserved.
Correct year?

[Mads Ager (chromium), 2008-11-25 21:09:41]

I went over the code looking for style issues.  I have not looked at what the
code does since this has all be reviewed in the branch.

http://codereview.chromium.org/12427/diff/1/29
File src/SConscript (right):

http://codereview.chromium.org/12427/diff/1/29#newcode53
Line 53: 'arch:arm':  ['assembler-arm.cc', 'builtins-arm.cc', 'codegen-arm.cc',
As pointed out by Ivan, there should be a new ARM file here and there should be
a split in an include that chooses the -ia32 or the -arm file.

http://codereview.chromium.org/12427/diff/1/23
File src/assembler-irregexp-inl.h (right):

http://codereview.chromium.org/12427/diff/1/23#newcode69
Line 69: void IrregexpAssembler::EmitOrLink(Label* l) {
Indentation is off here.  Use 2 spaces instead of 4.

http://codereview.chromium.org/12427/diff/1/13
File src/assembler-irregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/13#newcode43
Line 43: : buffer_(buffer),
4 space indent of the initializer list?

http://codereview.chromium.org/12427/diff/1/13#newcode235
Line 235: Label* on_mismatch) {
Identation is off here.

http://codereview.chromium.org/12427/diff/1/34
File src/assembler-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/34#newcode50
Line 50: void Break();  // This instruction will cause a fatal VM error if hit.
I would prefer to have this comment on the line before.

http://codereview.chromium.org/12427/diff/1/34#newcode52
Line 52: void Bind(Label* l);  // Binds an unbound label L to the current code
posn.
Ditto.

http://codereview.chromium.org/12427/diff/1/34#newcode75
Line 75: // the register after.  If a register contains -1 then the other
register
Two occurrences of 'the' after each other.  Also, reformatting the comment would
be nice.

http://codereview.chromium.org/12427/diff/1/34#newcode84
Line 84: // look up in a bit array, uses the result of that decide whether to
fall
decide -> to decide

http://codereview.chromium.org/12427/diff/1/34#newcode117
Line 117: IrregexpAssembler() { UNREACHABLE(); }
Can you use DISALLOW_IMPLICIT_CONSTRUCTORS(IrregexpAssembler) here (in other
words, should we hide default copy and assign as well)?  If you cannot, you
should leave out the implementation and just have IrregexpAssembler();  That way
you get link errors if you extend this with a friend class that accidentally
calls the default constructor.

http://codereview.chromium.org/12427/diff/1/34#newcode126
Line 126: int pc_;  // The program counter; moves forward.
Move this comment to the line before the declaration?

http://codereview.chromium.org/12427/diff/1/32
File src/assembler.h (right):

http://codereview.chromium.org/12427/diff/1/32#newcode54
Line 54: { Unuse(); }
This looks like an accidental edit?

http://codereview.chromium.org/12427/diff/1/43
File src/ast.cc (right):

http://codereview.chromium.org/12427/diff/1/43#newcode198
Line 198: FOR_EACH_REG_EXP_TREE_TYPE(MAKE_TYPE_CASE)
Do not indent this line.

http://codereview.chromium.org/12427/diff/1/20
File src/ast.h (right):

http://codereview.chromium.org/12427/diff/1/20#newcode1220
Line 1220: : alternatives_(alternatives) { }
Use four space indent of initializer list.  Multiple occurrences of this below
as well.

http://codereview.chromium.org/12427/diff/1/20#newcode1269
Line 1269: START_OF_LINE, START_OF_INPUT, END_OF_LINE, END_OF_INPUT,
One enum value per line?

http://codereview.chromium.org/12427/diff/1/20#newcode1364
Line 1364: CAPTURE_AVAILABLE, CAPTURE_UNREACHABLE,
CAPTURE_PERMANENTLY_UNREACHABLE };
Two space indent instead of four.  One enum value per line and the end brace on
a separate line.

http://codereview.chromium.org/12427/diff/1/52
File src/bytecodes-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/52#newcode34
Line 34: #define BYTECODE_ITERATOR(V)    \
Put the \ at the far right with the rest of them?

http://codereview.chromium.org/12427/diff/1/18
File src/flag-definitions.h (right):

http://codereview.chromium.org/12427/diff/1/18#newcode294
Line 294: DEFINE_bool(trace_regexp_bytecodes, false, "trace Irregexp bytecode
executon")
On 2008/11/25 11:03:52, Erik Corry wrote:
> speling eror

This one is still there.

http://codereview.chromium.org/12427/diff/1/36
File src/interpreter-irregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/36#newcode50
Line 50: pc - code_base,
Indentation one space off for all arguments except the first.

http://codereview.chromium.org/12427/diff/1/36#newcode62
Line 62: # define BYTECODE(name) case BC_##name:                                
      \
No space between # and define.

The indentation seems pretty strange.  Why not:

#define BYTECODE(name)                   \
  case BC_##name:                        \
    TraceInterpreter(...)

?

http://codereview.chromium.org/12427/diff/1/36#newcode70
Line 70: # define BYTECODE(name) case BC_##name:  // NOLINT
Remove space between # and define.

http://codereview.chromium.org/12427/diff/1/36#newcode336
Line 336: 
Remove one of the empty lines?

http://codereview.chromium.org/12427/diff/1/19
File src/interpreter-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/19#newcode28
Line 28: // A simple interpreter for the Regexp2000 byte code.
Regexp2000 -> regexp?

http://codereview.chromium.org/12427/diff/1/40
File src/jsregexp-inl.h (right):

http://codereview.chromium.org/12427/diff/1/40#newcode36
Line 36: namespace v8 {
For consistency, this should be 

namespace v8 { namespace internal {

http://codereview.chromium.org/12427/diff/1/40#newcode262
Line 262: }  // namespace internal
For consistency, this should be 

} }  // namespace v8::internal

http://codereview.chromium.org/12427/diff/1/48
File src/jsregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/48#newcode29
Line 29: #include <set>
On 2008/11/25 11:03:52, Erik Corry wrote:
> Where is this coming from?

These are still there.  Please remove!

http://codereview.chromium.org/12427/diff/1/48#newcode482
Line 482: {
Why is the extra scope needed here?

http://codereview.chromium.org/12427/diff/1/48#newcode493
Line 493: switch (tag) {
The indentation of this switch is off.  Indent the cases please.

http://codereview.chromium.org/12427/diff/1/48#newcode519
Line 519: default:
Why is this default case in the middle of the rest of the cases.  Please move it
to the end!

http://codereview.chromium.org/12427/diff/1/48#newcode603
Line 603: inline OffsetsVector(int num_registers) :
Move : to the next line and do a four space indent.

http://codereview.chromium.org/12427/diff/1/48#newcode643
Line 643: Handle<String> subject,
Indentation is off.

http://codereview.chromium.org/12427/diff/1/48#newcode741
Line 741: if (matches->IsNull()) {  // Exited loop normally.
Move these comments to new lines before the returns?

http://codereview.chromium.org/12427/diff/1/48#newcode808
Line 808: value();
This should fit on the previous line.

http://codereview.chromium.org/12427/diff/1/48#newcode920
Line 920: : next_register_(2 * (capture_count + 1)),
Four space indent.

http://codereview.chromium.org/12427/diff/1/48#newcode978
Line 978: } else {
Most of this nesting could go away since all of the cases return from the
function.  Simplify to:

if (a) {
  ...
  return true;
}

if (b) {
  ...
  return true;
}

...
return how_it_went;

?

http://codereview.chromium.org/12427/diff/1/48#newcode1005
Line 1005: break;
Indent the break.

http://codereview.chromium.org/12427/diff/1/48#newcode1008
Line 1008: break;
Indent the break.

http://codereview.chromium.org/12427/diff/1/48#newcode1148
Line 1148: if (((exor - 1) & exor) == 0) {
The if-else could be replaced by two ifs that each return from the function.

http://codereview.chromium.org/12427/diff/1/48#newcode1365
Line 1365: int i;
I don't see how the i is needed after the for loop.  There seems to be no breaks
out of the for loop or changes to choice_count during the loop, so we know the
value of i after the loop.  Move "int i" into the for loop and use "choise_count
- 1" instead of i after the loop?

http://codereview.chromium.org/12427/diff/1/48#newcode1547
Line 1547: : stream_(stream), choice_(choice) { }
Four space indent.

http://codereview.chromium.org/12427/diff/1/48#newcode1571
Line 1571: : first_(true), stream_(stream) { }
Four space indent.

http://codereview.chromium.org/12427/diff/1/48#newcode2086
Line 2086: && to() <= kRangeCanonicalizeMax) {
Indentation is off.

http://codereview.chromium.org/12427/diff/1/26
File src/jsregexp.h (right):

http://codereview.chromium.org/12427/diff/1/26#newcode187
Line 187: : from_(from),
Four space indent.  This migth all fit on one line though?

http://codereview.chromium.org/12427/diff/1/26#newcode279
Line 279: : key_(key),
Four space indent.  Many more occurrences below.

http://codereview.chromium.org/12427/diff/1/26#newcode283
Line 283: Key key() { return key_; }
Indentation is off for all the accessors.

http://codereview.chromium.org/12427/diff/1/26#newcode328
Line 328: private:
Have the empty line before private instead of after?

http://codereview.chromium.org/12427/diff/1/26#newcode355
Line 355: : from_(0), to_(0), out_set_(NULL) { }
Put this on one line.

http://codereview.chromium.org/12427/diff/1/26#newcode376
Line 376: if (a == b)
How about:

if (a == b) return 0;
if (a < b) return -1;
return 1;

http://codereview.chromium.org/12427/diff/1/26#newcode696
Line 696: Guard *guard,
On 2008/11/25 11:03:52, Erik Corry wrote:
> Misplaced asterisk.

This one is still there.

http://codereview.chromium.org/12427/diff/1/26#newcode741
Line 741: DispatchTable *table_;
On 2008/11/25 11:03:52, Erik Corry wrote:
> Misplaced asterisk.

This is still there.

http://codereview.chromium.org/12427/diff/1/12
File src/objects.cc (right):

http://codereview.chromium.org/12427/diff/1/12#newcode3508
Line 3508: : str_(str.location()),
Four space indent.  More occurrences below.

http://codereview.chromium.org/12427/diff/1/50
File src/parser.cc (right):

http://codereview.chromium.org/12427/diff/1/50#newcode235
Line 235: BufferedZoneList() :
All on one line.

http://codereview.chromium.org/12427/diff/1/50#newcode339
Line 339: : pending_empty_(false), characters_(NULL), terms_(), alternatives_()
Four space indent.  More occurrences below.

http://codereview.chromium.org/12427/diff/1/50#newcode4295
Line 4295: // Get multiline flag somehow
Is this a TODO or a comment on what the code is doing?

http://codereview.chromium.org/12427/diff/1/45
File src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/12427/diff/1/45#newcode81
Line 81: : masm_(new MacroAssembler(NULL, kRegExpCodeSize)),
Guess what?  :-)

http://codereview.chromium.org/12427/diff/1/3
File test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/12427/diff/1/3#newcode30
Line 30: #include <set>
Please remove.  We are not using <set>, right?

http://codereview.chromium.org/12427/diff/1/3#newcode75
Line 75: #define CHECK_ESCAPES(input, has_escapes) CHECK_EQ(has_escapes, \
How about:

#define CHECK_ESCAPES(input, has_escapes)    \
  CHECK_EQ(has_escapes, ParseEscapes(input))

[Christian Plesner Hansen, 2008-11-26 06:49:56]

I've fixed the issues that affect code I've written.  L&E, please do likewise
for your code.

http://codereview.chromium.org/12427/diff/1/29
File src/SConscript (right):

http://codereview.chromium.org/12427/diff/1/29#newcode48
Line 48: 'scopeinfo.cc', 'scopes.cc', 'serialize.cc', 'snapshot-common.cc',
Fixed

http://codereview.chromium.org/12427/diff/1/29#newcode58
Line 58: 'macro-assembler-ia32.cc', 'regexp-macro-assembler-ia32.cc',
Fixed

http://codereview.chromium.org/12427/diff/1/20
File src/ast.h (right):

http://codereview.chromium.org/12427/diff/1/20#newcode1220
Line 1220: : alternatives_(alternatives) { }
Fixed.

http://codereview.chromium.org/12427/diff/1/20#newcode1269
Line 1269: START_OF_LINE, START_OF_INPUT, END_OF_LINE, END_OF_INPUT,
Fixed.  Note, however, that the style guide doesn't say anything about this and
there are many other occurrences of having multiple enum values on the same
line.

http://codereview.chromium.org/12427/diff/1/40
File src/jsregexp-inl.h (right):

http://codereview.chromium.org/12427/diff/1/40#newcode36
Line 36: namespace v8 {
That was how it was everywhere originally but there seemed to be a general
feeling that it was too ugly.  If we want to be consistent I suggest we go the
other way.

http://codereview.chromium.org/12427/diff/1/48
File src/jsregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/48#newcode29
Line 29: #include <set>
It's in use.

http://codereview.chromium.org/12427/diff/1/48#newcode920
Line 920: : next_register_(2 * (capture_count + 1)),
Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode978
Line 978: } else {
Nesting aids readability: you can trivially see which conditions hold by looking
at the conditions of the enclosing if statements.  Otherwise you have to read
the conditions of the preceding if statements which may or may not be related to
the code that follows, and read their bodies to convince yourself that they do
indeed always return.

http://codereview.chromium.org/12427/diff/1/48#newcode1547
Line 1547: : stream_(stream), choice_(choice) { }
Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1571
Line 1571: : first_(true), stream_(stream) { }
Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode2086
Line 2086: && to() <= kRangeCanonicalizeMax) {
Fixed.

http://codereview.chromium.org/12427/diff/1/26
File src/jsregexp.h (right):

http://codereview.chromium.org/12427/diff/1/26#newcode187
Line 187: : from_(from),
Fixed.

http://codereview.chromium.org/12427/diff/1/26#newcode279
Line 279: : key_(key),
Fixed.

http://codereview.chromium.org/12427/diff/1/26#newcode283
Line 283: Key key() { return key_; }
Fixed.  I wonder why lint didn't catch this?

http://codereview.chromium.org/12427/diff/1/26#newcode328
Line 328: private:
Fixed.

http://codereview.chromium.org/12427/diff/1/26#newcode355
Line 355: : from_(0), to_(0), out_set_(NULL) { }
Fixed.

http://codereview.chromium.org/12427/diff/1/26#newcode376
Line 376: if (a == b)
That's a religious question: should there be an 'else' if the 'if' branch always
returns.  I believe there should, and then the guideline doesn't allow the
statement to be on the same line as the condition.

http://codereview.chromium.org/12427/diff/1/12
File src/objects.cc (right):

http://codereview.chromium.org/12427/diff/1/12#newcode3508
Line 3508: : str_(str.location()),
Fixed.

http://codereview.chromium.org/12427/diff/1/27
File src/objects.h (right):

http://codereview.chromium.org/12427/diff/1/27#newcode2130
Line 2130: NUMBER_OF_KINDS = KEYED_STORE_IC + 1
Should be LAST_IC_KIND

http://codereview.chromium.org/12427/diff/1/3
File test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/12427/diff/1/3#newcode1
Line 1: // Copyright 2006-2008 the V8 project authors. All rights reserved.
Fixed

http://codereview.chromium.org/12427/diff/1/3#newcode30
Line 30: #include <set>
Yes we are.

[iposva, 2008-11-26 06:56:22]

More comments being piled on here.

In general a lot of the new classes are missing comments explaining what the
code is trying to do. I did grow tired of adding comments for every class where
comments were missing. Same is true for the declaration order within the class.

Methods would also not suffer basic explanation what they are trying to achieve.

Thanks,
-Ivan

http://codereview.chromium.org/12427/diff/1/34
File src/assembler-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/34#newcode131
Line 131: void Expand();
Please move the method definition before the fields to be consistent with the
C++ style guide:
  Typedefs and Enums
  Constants
  Constructors
  Destructor
  Methods, including static methods
  Data Members, including static data members

http://codereview.chromium.org/12427/diff/1/26
File src/jsregexp.h (right):

http://codereview.chromium.org/12427/diff/1/26#newcode181
Line 181: class CharacterRange {
Please add comments about this class.

http://codereview.chromium.org/12427/diff/1/26#newcode209
Line 209: static const int kRangeCanonicalizeMax = 0x200;
Declaration order: constants before constructors.
Blank lines separating the sections would help too when telling these sections
apart.

http://codereview.chromium.org/12427/diff/1/26#newcode212
Line 212: private:
Please leave a blank line before "private:" in accordance to improve readability
and highlight the break.

http://codereview.chromium.org/12427/diff/1/15
File src/regexp-macro-assembler-ia32.h (right):

http://codereview.chromium.org/12427/diff/1/15#newcode31
Line 31: #if !(defined __arm__ || defined __thumb__ || defined ARM)
Checking for ARM does not belong into this file. It should never be included in
an ARM build.

http://codereview.chromium.org/12427/diff/1/15#newcode38
Line 38: class RegExpMacroAssemblerIA32: public RegExpMacroAssembler {
Kevin and I found that naming a class with MyClassIA32 or MyClassARM actually
makes things much more complicated. We are not set up to generate code for IA32
and ARM within one binary so that distinction is not providing much benefit, but
allows shared code to just refer to MyClass.
Also only having one implementation allowed us to devirtualize the methods. I
would suggest to take a look at how the CodeGenerator is setup. The small
disadvantage is that for truly shared methods you have to declare them for each
architecture. The definition can still be in the shared source file.

http://codereview.chromium.org/12427/diff/1/35
File src/utils.h (right):

http://codereview.chromium.org/12427/diff/1/35#newcode87
Line 87: static int Spaceship(const T& a, const T& b) {
Comments please. What kind of spaceship is this? To me this looks like a
comparison, but what do I know?

http://codereview.chromium.org/12427/diff/1/35#newcode511
Line 511: static inline int Load16(const byte* pc) {
I understand that this is most likely used when accessing multi-byte values in
generated code or bytecodes, but using ptr insted of pc would probably be more
appropriate.

[Christian Plesner Hansen, 2008-11-26 07:49:51]

http://codereview.chromium.org/12427/diff/1/26
File src/jsregexp.h (right):

http://codereview.chromium.org/12427/diff/1/26#newcode209
Line 209: static const int kRangeCanonicalizeMax = 0x200;
We've never followed that rule.  In fact, as far as I can tell we've
consistently done the opposite and put constants after constructors.

http://codereview.chromium.org/12427/diff/1/26#newcode212
Line 212: private:
Again, that's not a rule we've been consistent about before.

http://codereview.chromium.org/12427/diff/1/35
File src/utils.h (right):

http://codereview.chromium.org/12427/diff/1/35#newcode87
Line 87: static int Spaceship(const T& a, const T& b) {
I'll rename it.  The name "spaceship" is apparently not as well known as I
thought.  http://en.wikipedia.org/wiki/Spaceship_operator

[Erik Corry, 2008-11-26 12:18:36]

I've gone through the comments for the code I feel responsible for.

http://codereview.chromium.org/12427/diff/1/23
File src/assembler-irregexp-inl.h (right):

http://codereview.chromium.org/12427/diff/1/23#newcode69
Line 69: void IrregexpAssembler::EmitOrLink(Label* l) {
On 2008/11/25 21:09:41, Mads Ager wrote:
> Indentation is off here.  Use 2 spaces instead of 4.

Fixed.

http://codereview.chromium.org/12427/diff/1/13
File src/assembler-irregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/13#newcode43
Line 43: : buffer_(buffer),
On 2008/11/25 21:09:41, Mads Ager wrote:
> 4 space indent of the initializer list?

Fixed.

http://codereview.chromium.org/12427/diff/1/13#newcode235
Line 235: Label* on_mismatch) {
On 2008/11/25 21:09:41, Mads Ager wrote:
> Identation is off here.

Fixed.

http://codereview.chromium.org/12427/diff/1/34
File src/assembler-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/34#newcode50
Line 50: void Break();  // This instruction will cause a fatal VM error if hit.
On 2008/11/25 21:09:41, Mads Ager wrote:
> I would prefer to have this comment on the line before.

Fixed.

http://codereview.chromium.org/12427/diff/1/34#newcode52
Line 52: void Bind(Label* l);  // Binds an unbound label L to the current code
posn.
On 2008/11/25 21:09:41, Mads Ager wrote:
> Ditto.

Fixed.

http://codereview.chromium.org/12427/diff/1/34#newcode75
Line 75: // the register after.  If a register contains -1 then the other
register
On 2008/11/25 21:09:41, Mads Ager wrote:
> Two occurrences of 'the' after each other.  Also, reformatting the comment
would
> be nice.

Fixed

http://codereview.chromium.org/12427/diff/1/34#newcode84
Line 84: // look up in a bit array, uses the result of that decide whether to
fall
On 2008/11/25 21:09:41, Mads Ager wrote:
> decide -> to decide

Fixed

http://codereview.chromium.org/12427/diff/1/34#newcode117
Line 117: IrregexpAssembler() { UNREACHABLE(); }
On 2008/11/25 21:09:41, Mads Ager wrote:
> Can you use DISALLOW_IMPLICIT_CONSTRUCTORS(IrregexpAssembler) here (in other
> words, should we hide default copy and assign as well)?  If you cannot, you
> should leave out the implementation and just have IrregexpAssembler();  That
way
> you get link errors if you extend this with a friend class that accidentally
> calls the default constructor.

Fixed.

http://codereview.chromium.org/12427/diff/1/34#newcode126
Line 126: int pc_;  // The program counter; moves forward.
On 2008/11/25 21:09:41, Mads Ager wrote:
> Move this comment to the line before the declaration?

I blame the Strongtalk team.

Fixed.  Also removed the 'moves forward'.  What else is it going to do?

http://codereview.chromium.org/12427/diff/1/34#newcode131
Line 131: void Expand();
On 2008/11/26 06:56:22, iposva wrote:
> Please move the method definition before the fields to be consistent with the
> C++ style guide:
>   Typedefs and Enums
>   Constants
>   Constructors
>   Destructor
>   Methods, including static methods
>   Data Members, including static data members
> 
> 

I moved this method but haven't reordered the entire file, since doing so would
be inconsistent with the rest of V8.

http://codereview.chromium.org/12427/diff/1/32
File src/assembler.h (right):

http://codereview.chromium.org/12427/diff/1/32#newcode54
Line 54: { Unuse(); }
On 2008/11/25 21:09:41, Mads Ager wrote:
> This looks like an accidental edit?

Indeed it does.  Fixed.

http://codereview.chromium.org/12427/diff/1/43
File src/ast.cc (right):

http://codereview.chromium.org/12427/diff/1/43#newcode198
Line 198: FOR_EACH_REG_EXP_TREE_TYPE(MAKE_TYPE_CASE)
On 2008/11/25 21:09:41, Mads Ager wrote:
> Do not indent this line.

Fixed.

http://codereview.chromium.org/12427/diff/1/52
File src/bytecodes-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/52#newcode34
Line 34: #define BYTECODE_ITERATOR(V)    \
On 2008/11/25 21:09:41, Mads Ager wrote:
> Put the \ at the far right with the rest of them?

Fixed.

http://codereview.chromium.org/12427/diff/1/18
File src/flag-definitions.h (right):

http://codereview.chromium.org/12427/diff/1/18#newcode294
Line 294: DEFINE_bool(trace_regexp_bytecodes, false, "trace Irregexp bytecode
executon")
On 2008/11/25 21:09:41, Mads Ager wrote:
> On 2008/11/25 11:03:52, Erik Corry wrote:
> > speling eror
> 
> This one is still there.

Fixed.

http://codereview.chromium.org/12427/diff/1/36
File src/interpreter-irregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/36#newcode50
Line 50: pc - code_base,
On 2008/11/25 21:09:41, Mads Ager wrote:
> Indentation one space off for all arguments except the first.

Fixed.

http://codereview.chromium.org/12427/diff/1/36#newcode62
Line 62: # define BYTECODE(name) case BC_##name:                                
      \
On 2008/11/25 21:09:41, Mads Ager wrote:
> No space between # and define.

I like indenting #if in an analogous way to the indentation of if.  However it
is now fixed.

> The indentation seems pretty strange.  Why not:
> 
> #define BYTECODE(name)                   \
>   case BC_##name:                        \
>     TraceInterpreter(...)
> 
> ?

Fixed.

http://codereview.chromium.org/12427/diff/1/36#newcode70
Line 70: # define BYTECODE(name) case BC_##name:  // NOLINT
On 2008/11/25 21:09:41, Mads Ager wrote:
> Remove space between # and define.

Fixed.

http://codereview.chromium.org/12427/diff/1/36#newcode336
Line 336: 
On 2008/11/25 21:09:41, Mads Ager wrote:
> Remove one of the empty lines?

Fixed.

http://codereview.chromium.org/12427/diff/1/19
File src/interpreter-irregexp.h (right):

http://codereview.chromium.org/12427/diff/1/19#newcode28
Line 28: // A simple interpreter for the Regexp2000 byte code.
On 2008/11/25 21:09:41, Mads Ager wrote:
> Regexp2000 -> regexp?

No:  Regexp2000 -> Irregexp.

Fixed.

http://codereview.chromium.org/12427/diff/1/48
File src/jsregexp.cc (right):

http://codereview.chromium.org/12427/diff/1/48#newcode482
Line 482: {
On 2008/11/25 21:09:41, Mads Ager wrote:
> Why is the extra scope needed here?

It isn't.  Removed.

http://codereview.chromium.org/12427/diff/1/48#newcode493
Line 493: switch (tag) {
On 2008/11/25 21:09:41, Mads Ager wrote:
> The indentation of this switch is off.  Indent the cases please.

Done.

http://codereview.chromium.org/12427/diff/1/48#newcode519
Line 519: default:
On 2008/11/25 21:09:41, Mads Ager wrote:
> Why is this default case in the middle of the rest of the cases.  Please move
it
> to the end!

Because it falls through?  Lasse, please take a look at this, perhaps it is
correct.

http://codereview.chromium.org/12427/diff/1/48#newcode643
Line 643: Handle<String> subject,
On 2008/11/25 21:09:41, Mads Ager wrote:
> Indentation is off.

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode741
Line 741: if (matches->IsNull()) {  // Exited loop normally.
On 2008/11/25 21:09:41, Mads Ager wrote:
> Move these comments to new lines before the returns?

I'm not sure why that's any better, but fixed.  While I was at it I fixed the
same code in JSCREExecGlobal.  Why should only new code get the benefit of
arbitrary reformatting?

http://codereview.chromium.org/12427/diff/1/48#newcode808
Line 808: value();
On 2008/11/25 21:09:41, Mads Ager wrote:
> This should fit on the previous line.

Done.

http://codereview.chromium.org/12427/diff/1/48#newcode840
Line 840: // New regular expression engine
On 2008/11/25 11:03:52, Erik Corry wrote:
> Probably should mention "Irregexp"

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1005
Line 1005: break;
On 2008/11/25 21:09:41, Mads Ager wrote:
> Indent the break.

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1008
Line 1008: break;
On 2008/11/25 21:09:41, Mads Ager wrote:
> Indent the break.

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1148
Line 1148: if (((exor - 1) & exor) == 0) {
On 2008/11/25 21:09:41, Mads Ager wrote:
> The if-else could be replaced by two ifs that each return from the function.

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1228
Line 1228: Label *char_is_in_class =
On 2008/11/25 11:03:52, Erik Corry wrote:
> Misplaced asterisk.

Fixed.

http://codereview.chromium.org/12427/diff/1/48#newcode1365
Line 1365: int i;
On 2008/11/25 21:09:41, Mads Ager wrote:
> I don't see how the i is needed after the for loop.  There seems to be no
breaks
> out of the for loop or changes to choice_count during the loop, so we know the
> value of i after the loop.  Move "int i" into the for loop and use
"choise_count
> - 1" instead of i after the loop? 

Fixed.

http://codereview.chromium.org/12427/diff/1/50
File src/parser.cc (right):

http://codereview.chromium.org/12427/diff/1/50#newcode4295
Line 4295: // Get multiline flag somehow
On 2008/11/25 21:09:41, Mads Ager wrote:
> Is this a TODO or a comment on what the code is doing?

It's a TODO.  Fixed.

http://codereview.chromium.org/12427/diff/1/15
File src/regexp-macro-assembler-ia32.h (right):

http://codereview.chromium.org/12427/diff/1/15#newcode38
Line 38: class RegExpMacroAssemblerIA32: public RegExpMacroAssembler {
On 2008/11/26 06:56:22, iposva wrote:
> Kevin and I found that naming a class with MyClassIA32 or MyClassARM actually
> makes things much more complicated. We are not set up to generate code for
IA32
> and ARM within one binary so that distinction is not providing much benefit,
but
> allows shared code to just refer to MyClass.
> Also only having one implementation allowed us to devirtualize the methods. I
> would suggest to take a look at how the CodeGenerator is setup. The small
> disadvantage is that for truly shared methods you have to declare them for
each
> architecture. The definition can still be in the shared source file.

I'm not a great fan of having two completely different classes with the same
name but only putting one of them in a given binary.  In my experience it makes
changes difficult and confusing.

In this case there is a third class, the bytecode macro assembler, so the issue
of having more than one in the same binary is a real one.  There is accordingly
no chance of removing virtuals and there is a common superclass for referring to
an unknown subclass in shared code.

http://codereview.chromium.org/12427/diff/1/35
File src/utils.h (right):

http://codereview.chromium.org/12427/diff/1/35#newcode511
Line 511: static inline int Load16(const byte* pc) {
On 2008/11/26 06:56:22, iposva wrote:
> I understand that this is most likely used when accessing multi-byte values in
> generated code or bytecodes, but using ptr insted of pc would probably be more
> appropriate.

Yes, that's what it's used for and in this context it has the correct name.

[Erik Corry, 2008-11-26 12:33:33]

I'm afraid I don't like parts of this change.  The new RegExpMacroAssemblerImpl
typedef is only used in one place and that's inside an ifdef.  It only adds
confusion.  The new include file does something very simple and is used twice
where it could be inlined.

Modulo this and the other comments it LGTM