Last Comment Bug 676738 - Change jsint index arguments to JS_*Element functions to uint32 index
: Change jsint index arguments to JS_*Element functions to uint32 index
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 586842
  Show dependency treegraph
 
Reported: 2011-08-04 18:22 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-09 08:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change JS_DefineElement and adjust users (6.45 KB, patch)
2011-08-04 18:24 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
jst: review+
Details | Diff | Splinter Review
Change JS_AlreadyHasOwnElement (1.35 KB, patch)
2011-08-04 18:25 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review
Change JS_HasElement (2.29 KB, patch)
2011-08-04 18:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review
Change JS_LookupElement (1.24 KB, patch)
2011-08-04 18:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review
Change JS_GetElement (2.71 KB, patch)
2011-08-04 18:28 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review
Change JS_SetElement (4.54 KB, patch)
2011-08-04 18:29 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review
Change JS_DeleteElement{,2} (1.69 KB, patch)
2011-08-04 18:30 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:22:08 PDT
Sparse/dense property storage will want this API split to make indexed accesses more efficient.

I queried the newsgroup about making the change (versus adding yet another kind of property-access API), and no one expressed concern.  It's also worth noting that every use of these methods that I found in mozilla-central was using only the non-negative half of the index space.  I expect this is the common case.  I wonder, even, how much anyone ever used negative elements -- I'd guess element stuff is mostly used for arrays and arraylike things, and you're not going to hit negative elements there.

(I did end up changing some Mozilla callers to avoid possible compiler warnings.  Technically I probably could have gotten away without doing this, but it seemed a good time to clean up signage usage in that code when I already had to audit them to make sure none depended on being able to pass negative indexes.)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:24:46 PDT
Created attachment 550917 [details] [diff] [review]
Change JS_DefineElement and adjust users

I'm doing one patch per method changed, to keep things bite-sized.  There's no particular reason it couldn't all be concatenated together into one patch, it just might get (a little bit) big.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:25:53 PDT
Created attachment 550919 [details] [diff] [review]
Change JS_AlreadyHasOwnElement
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:26:43 PDT
Created attachment 550920 [details] [diff] [review]
Change JS_HasElement
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:27:22 PDT
Created attachment 550921 [details] [diff] [review]
Change JS_LookupElement
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:28:32 PDT
Created attachment 550922 [details] [diff] [review]
Change JS_GetElement
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:29:35 PDT
Created attachment 550923 [details] [diff] [review]
Change JS_SetElement
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 18:30:05 PDT
Created attachment 550924 [details] [diff] [review]
Change JS_DeleteElement{,2}

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