JS regexp trivial cleanups

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
trivial
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Brian Crowder, Assigned: Brian Crowder)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

12 years ago
Patch next.
(Assignee)

Comment 1

12 years ago
Created attachment 237823 [details] [diff] [review]
Removing three unused opcodes.
Attachment #237823 - Flags: review?(mrbkap)
Comment on attachment 237823 [details] [diff] [review]
Removing three unused opcodes.

We don't have to bump the bytecode version because the used bytecodes stay the same, right? And we don't have to worry about old XDR'd bytecodes because those will be stopped by the new bytecode versioning scheme.
Attachment #237823 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 3

12 years ago
Actually, I'm just about to submit a larger set of changes which might require bumping the bytecode version.  Are we attached to this style of explicitly assigning the bytecode values in their enum?  I would like to ditch that so that I can reorder the bytecodes or add new bytecodes a little more flexibly.
Regular expressions are XDR'ed by source, not by bytecode, so we do not need to version bytecode.

Nor do we need to give REOp enumerators explicit values (I try to do that only for external data formats, but sometimes overdo it; in this case it may be that rogerl was imitating the style I used in the old, Netscape 4 era NFA).

/be
(Assignee)

Comment 5

12 years ago
Great, thanks Brendan.  Bigger patch shortly (with REOP_IS_SIMPLE() made simpler, no obviously unused opcodes, and no explicit enumerating).
(Assignee)

Comment 6

12 years ago
Created attachment 237864 [details] [diff] [review]
More changes

I will change the scope of this bug to be something like "general js regexp cleanup".  This patch does the following:

* REOp enums aren't explicitly numbered anymore
* Some ops have been reordered, all "simple" are ordered first (slightly less math done when determining if an op is simple), EMPTY is now a simple op
* I added a REOP_MAXOP and sprinkled some assertions about that have helped me debug bad behavior in traversing programs (not original bugs, stuff I broke)
* REOP_END occurs earlier in the switch now, since I wanted it to be a "simple" op, but it can't be without introducing mess (needs to return from ExecuteREBytecode immediately).  Grouping it at the stop of the switch at least keeps it from being lost in the more complex stuff below.

This patch looks bigger than it is.  I ran a small section of the test suite (my favorite regexp- and regexp-regression- tests) and they work as before.
Attachment #237823 - Attachment is obsolete: true
Attachment #237864 - Flags: review?(mrbkap)
(Assignee)

Comment 7

12 years ago
Comment on attachment 237864 [details] [diff] [review]
More changes

Wait, not ready for review yet
Attachment #237864 - Flags: review?(mrbkap)
(Assignee)

Comment 8

12 years ago
Created attachment 237865 [details] [diff] [review]
Incorporating earlier patch also

Forgot to remove the ops removed in the earlier patch also.
Attachment #237864 - Attachment is obsolete: true
(Assignee)

Comment 9

12 years ago
Comment on attachment 237865 [details] [diff] [review]
Incorporating earlier patch also

Read for review now  :)
Attachment #237865 - Flags: review?(mrbkap)
(Assignee)

Updated

12 years ago
Summary: JS regexp has unused opcodes → JS regexp trivial cleanups
(Assignee)

Comment 10

12 years ago
mrbkap:  Can you re-review this?  New patch is a bit larger.
Comment on attachment 237865 [details] [diff] [review]
Incorporating earlier patch also

If brendan's OK with the crazy comma style, r=mrbkap
Attachment #237865 - Flags: superreview?(brendan)
Attachment #237865 - Flags: review?(mrbkap)
Attachment #237865 - Flags: review+
Comment on attachment 237865 [details] [diff] [review]
Incorporating earlier patch also

>Index: jsregexp.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsregexp.c,v
>retrieving revision 3.124
>diff -u -8 -p -r3.124 jsregexp.c
>--- jsregexp.c	1 Aug 2006 20:00:03 -0000	3.124
>+++ jsregexp.c	12 Sep 2006 00:11:02 -0000
>@@ -60,75 +60,70 @@
> #include "jsobj.h"
> #include "jsopcode.h"
> #include "jsregexp.h"
> #include "jsscan.h"
> #include "jsstr.h"
> 
> /* Note : contiguity of 'simple opcodes' is important for SimpleMatch() */
> typedef enum REOp {
>-    REOP_EMPTY         = 0,  /* match rest of input against rest of r.e. */

In vim, sit on this line and type :.,.+60s/\(  *\),/,\1/

REOP_SIMPLE_END could be equated with the enum before it, or just get rid of it and wire that last simple op's name into the macro.  Over-abstracting doesn't help at this scale.

/be
(Assignee)

Comment 13

12 years ago
Created attachment 237908 [details] [diff] [review]
Crazy commas gone

Also, improved the case label and default indentation for switch statements to match house style.
Attachment #237865 - Attachment is obsolete: true
Attachment #237908 - Flags: review?(mrbkap)
Attachment #237865 - Flags: superreview?(brendan)
Comment on attachment 237908 [details] [diff] [review]
Crazy commas gone

I'm hoping the diff -w looks clean.
Attachment #237908 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 15

12 years ago
Created attachment 237912 [details] [diff] [review]
Half-indenting a couple labels I missed

There are some other labels not half-indented here, but they don't disagree with house-style AS overtly as other stuff.  I will probably touch them as I fidget with more regexp code in the future.  For now they're fine.
Attachment #237908 - Attachment is obsolete: true
Attachment #237912 - Flags: review?(mrbkap)
(Assignee)

Comment 16

12 years ago
Created attachment 237913 [details] [diff] [review]
White-space changes removed for review
(Assignee)

Comment 17

12 years ago
Comment on attachment 237913 [details] [diff] [review]
White-space changes removed for review

Marking as obsolete for clarity
Attachment #237913 - Attachment is obsolete: true
Comment on attachment 237912 [details] [diff] [review]
Half-indenting a couple labels I missed

Cool.
Attachment #237912 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

12 years ago
Attachment #237912 - Flags: superreview?(brendan)
Comment on attachment 237912 [details] [diff] [review]
Half-indenting a couple labels I missed


>+    REOP_NCLASS,        /* negated character class with index */
>+    /* NCLASS is considered to be the last "simple" op-code */

A newline before and after the /* NCLASS is considered... */ comment would be nicer layout.

>+    REOP_MAXOP          /* META: no operator higher than this */

Or equal to this.  Suggest REOP_LIMIT for fencepost value, i.e., do not equal or exceed.  MAX is used to mean maximum valid code.

sr=me with these nits picked.

/be
Attachment #237912 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 20

12 years ago
Created attachment 237919 [details] [diff] [review]
Nits dutifully picked.
Attachment #237912 - Attachment is obsolete: true
Comment on attachment 237919 [details] [diff] [review]
Nits dutifully picked.

Thanks!  Nit-picking is good for the soul.  BTW, feel free to attach diff -w for review only but not obsolete it -- just say FOR REVIEW ONLY (or the same, but not shouting ;-).

/be
Attachment #237919 - Flags: superreview+
Attachment #237919 - Flags: review+
(Assignee)

Comment 22

12 years ago
Created attachment 237925 [details] [diff] [review]
FOR REVIEW ONLY without the shouting
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
mozilla/js/src/jsregexp.c 	3.125
Status: NEW → RESOLVED
Last Resolved: 12 years ago
OS: Mac OS X 10.3 → All
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.