As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 1043683 - Make RegExpStatics::makeMatch sane about not taking two arguments, one of which is always double the other (yet has its parity tested)
: Make RegExpStatics::makeMatch sane about not taking two arguments, one of whi...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript: Standard Library (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla34
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 369778
  Show dependency treegraph
 
Reported: 2014-07-24 16:15 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2014-08-13 16:18 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard: [qa-]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 (2.59 KB, patch)
2014-07-24 16:15 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
ae.anderson0: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-07-24 16:15:11 PDT
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.
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-07-24 16:15:21 PDT
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.  :-)
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-07-24 16:17:42 PDT
(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 User image Andy Anderson 2014-07-24 21:31:42 PDT
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.
Comment 4 User image Andy Anderson 2014-07-24 21:49:07 PDT
(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.
Comment 5 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-07-25 16:35:20 PDT
Thanks!  Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5edaa0a67cb8
Comment 6 User image Carsten Book [:Tomcat] 2014-07-28 06:38:18 PDT
https://hg.mozilla.org/mozilla-central/rev/5edaa0a67cb8

Note You need to log in before you can comment on or make changes to this bug.