The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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
Created 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

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 3

3 years ago
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+

Comment 4

3 years ago
(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.
Thanks!  Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5edaa0a67cb8
https://hg.mozilla.org/mozilla-central/rev/5edaa0a67cb8
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.