Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(335)

Issue 13783: * Made preemption work in Irregexp-native-ia32 (Closed)

Created:
12 years ago by Lasse Reichstein
Modified:
9 years, 6 months ago
Reviewers:
Erik Corry
CC:
v8-dev
Visibility:
Public.

Description

Preemption code for irregexp-native-ia32. Regexps can not only succeede or fail, but also report a thrown exception.

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+469 lines, -288 lines) Patch
M src/assembler-ia32.h View 1 5 chunks +27 lines, -18 lines 0 comments Download
M src/assembler-ia32.cc View 1 chunk +0 lines, -23 lines 0 comments Download
M src/assembler-ia32-inl.h View 1 2 chunks +22 lines, -0 lines 0 comments Download
M src/execution.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M src/execution.cc View 1 2 chunks +66 lines, -0 lines 0 comments Download
M src/jsregexp.cc View 1 7 chunks +22 lines, -7 lines 0 comments Download
M src/regexp-macro-assembler-ia32.h View 5 chunks +36 lines, -9 lines 0 comments Download
M src/regexp-macro-assembler-ia32.cc View 1 10 chunks +147 lines, -66 lines 0 comments Download
M src/runtime.cc View 2 chunks +2 lines, -63 lines 0 comments Download
M test/cctest/test-regexp.cc View 1 16 chunks +141 lines, -102 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
Lasse Reichstein
Major review. Suggestions for alternative location or name for Execution::HandleStackGuardInterrup appreciated.
12 years ago (2008-12-12 08:53:20 UTC) #1
Erik Corry
12 years ago (2008-12-12 10:01:05 UTC) #2
LGTM.

We need to devise a test for GC-during-regexp-matching.

Does it lint?

Does it run on Mac?

http://codereview.chromium.org/13783/diff/1/4
File src/assembler-ia32.h (right):

http://codereview.chromium.org/13783/diff/1/4#newcode187
Line 187: inline explicit Immediate(Label* value);
I think this is rather confusing.  It's for emitting an offset to the start of
the code object, but it looks like it's for emitting the address of the label
itself.  Since constructors can't be renamed I'd prefer a static method that
returns the result of a private constructor.  That way you can call it
CodeObjectRelativeOffset(Label*).

http://codereview.chromium.org/13783/diff/1/4#newcode753
Line 753: inline void emit_label(Label* label);
The name of this is very generic.  It should have a longer name that reflects
what it does, eg. emit_code_relative_offset.

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

http://codereview.chromium.org/13783/diff/1/5#newcode178
Line 178: INTERNAL_REFERENCE,  // An address inside the same function.
I liked it better before.

http://codereview.chromium.org/13783/diff/1/7
File src/execution.h (right):

http://codereview.chromium.org/13783/diff/1/7#newcode124
Line 124: // stack overflow, the handle the interruption.
this sense make no

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

http://codereview.chromium.org/13783/diff/1/9#newcode198
Line 198: switch (mode_) {
I liked it better before with if instead of switch.  Since there are only two
possibilities you might as well tell the compiler and the reader that.  One of
them can optimize better and the other can understand better.

http://codereview.chromium.org/13783/diff/1/9#newcode215
Line 215: switch (mode_) {
Ditto and more places below.

Powered by Google App Engine
This is Rietveld 408576698