Closed Bug 1043683 Opened 10 years ago Closed 10 years ago

Make RegExpStatics::makeMatch sane about not taking two arguments, one of which is always double the other (yet has its parity tested)

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(1 file)

In the process of reading bug 369778's patch I realized RegExpStatics::makeMatch is kind of stupid, and we can remove an argument and save substantially on complexity in the process.  We should do that.
Blocks: 369778
Andy, seeing as this is a relatively simple change what with makeMatch being used only in this file, and seeing as you've looked at this code most recently, you're probably the best person to review this, seems to me.  It'll bitrot your patch slightly, but I can review around the changes.  :-)
Attachment #8462097 - Flags: review?(ae.anderson0)
(Protocol on review requests, if you're not yet familiar with it: set the review flag to + if you think the patch is good, or set it to - if you think it's bad.  If you have comments on anything -- better ways to do one thing or another, suggestions for better comment wording, etc. -- feel free to make them in the process.)
Comment on attachment 8462097 [details] [diff] [review]
Make RegExpStatics::makeMatch sane about not taking two arguments, one of which is always double the other (yet has its parity tested).  NOT REVIEWED YET

Review of attachment 8462097 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine to me. I'd like it if there were an additional comment demanding parity on the input, but given that this code shouldn't move around too much it's not vital.
Attachment #8462097 - Flags: review?(ae.anderson0) → review+
(In reply to Andy Anderson from comment #3)
> Comment on attachment 8462097 [details] [diff] [review]
> Make RegExpStatics::makeMatch sane about not taking two arguments, one of
> which is always double the other (yet has its parity tested).  NOT REVIEWED
> YET
> 
> Review of attachment 8462097 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems fine to me. I'd like it if there were an additional comment demanding
> parity on the input, but given that this code shouldn't move around too much
> it's not vital.

It's late. Patch is fine as is, ignore my earlier comments suggesting otherwise.
https://hg.mozilla.org/mozilla-central/rev/5edaa0a67cb8
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: