563575 - encapsulate jsobj.cpp dslots accesses

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

his patch removes all but one (which I'll do later) of the non-encapsulated uses of dslots from jsobj.cpp.  Notable things:

- Adds addressOfDenseArrayElement().

- SLOTS_TO_DYNAMIC_WORDS and DYNAMIC_WORDS_TO_SLOTS were changed to JSObject
  inline methods.
  
- js_AllocSlots(), js_GrowSlots() and js_ShrinkSlots() were pulled in as 
  JSObject methods.
  
- I replaced this idiom:
  
    *slots++ = nslots;
    
  with this:
  
    slots++;
    slots[-1] = nslots;
    
  which is clearer and will make later changes to the meaning of slots[-1] 
  easier.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 443275 [details] [diff] [review]
patch

>+    jsval *tmpdslots = dslots;
>+    tmpdslots = (jsval*) cx->realloc(dslots - 1, nwords * sizeof(jsval));

First set of tmpdslots is useless -- lose it.

>+    if (!tmpdslots)
>+        return false;
>+
>+    dslots = tmpdslots;

The tmpdslots name here is a bit off. Not sure what to suggest -- "dslotsvec"?

/be

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> (From update of attachment 443275 [details] [diff] [review])
> >+    jsval *tmpdslots = dslots;
> >+    tmpdslots = (jsval*) cx->realloc(dslots - 1, nwords * sizeof(jsval));
> 
> First set of tmpdslots is useless -- lose it.
> 
> >+    if (!tmpdslots)
> >+        return false;
> >+
> >+    dslots = tmpdslots;
> 
> The tmpdslots name here is a bit off. Not sure what to suggest -- "dslotsvec"?

It feels spot on to me -- you can't use dslots directly because if realloc() fails you will have trashed the old dslots value.  So you need to use a temporary.  Hence tmpdslots.

Comment 3

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/187831aa97ac

Comment 4

[Brendan Eich [:brendan]]

tmpdslots is ok -- I've used names like that and will again -- what I was getting at was that it never is advanced such that tmpdslots[-1] is the length. So could it have a different name to connote the lack of the +1 bias? No big.

/be

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/187831aa97ac