Closed Bug 887563 Opened 6 years ago Closed 6 years ago

Convert CallArgs::operator[] to return a Handle

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(5 files)

We have almost as many call sites using handleAt() as operator[]. Since MutableHandleValue will automatically unpack to HandleValue and from there to a Value, it is straightforward to change operator[] to return a MutalbeHandle and rip out handleAt.
Attachment #768062 - Flags: review?(jwalden+bmo)
Trivial mechanical replacement of handleAt with operator[].
Attachment #768066 - Flags: review?(jwalden+bmo)
It turns out this just compiles. Next patch will remove the duplicate handleOrUndefinedAt.
Attachment #768069 - Flags: review?(jwalden+bmo)
And switches callers over to the now-identical ::get method.

I think there is going to be one more patch in this series to fix up anything in the browser that I just broke.
Attachment #768071 - Flags: review?(jwalden+bmo)
Comment on attachment 768062 [details] [diff] [review]
Part 1 - convert operator[] to MutableHandleValue

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

::: js/src/vm/SelfHosting.cpp
@@ +472,5 @@
>      JS_ASSERT(args.length() == 3);
>      JS_ASSERT(args[0].isObject());
>      JS_ASSERT(args[1].isInt32());
>  
> +    args[0].toObject().setReservedSlot(args[1].get().toPrivateUint32(), args[2]);

Should we add toPrivateUint32() on HandleValue too?
Please fix JSJit*CallArgs to continue matching CallArgs.
And chances are you didn't break anything in the browser: the only handleAt consumers I see in browser are using the jit callargs structs.
Comment on attachment 768062 [details] [diff] [review]
Part 1 - convert operator[] to MutableHandleValue

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

::: js/src/jsiter.cpp
@@ -755,5 @@
>      if (args.length() >= 2)
>          keyonly = ToBoolean(args[1]);
>      unsigned flags = JSITER_OWNONLY | (keyonly ? 0 : (JSITER_FOREACH | JSITER_KEYVALUE));
>  
> -    if (!ValueToIterator(cx, flags, MutableHandleValue::fromMarkedLocation(&args[0])))

fromMarkedLocation--!

::: js/src/vm/Debugger.cpp
@@ +4136,5 @@
>  }
>  
>  static JSBool
>  DebuggerGenericEval(JSContext *cx, const char *fullMethodName,
> +                    const Value &code, bool evalWithBindings, HandleValue bindings,

Instead of a bool that shows up in call sites with no explanation, make this an enum of some sort.  Maybe:

enum EvalBindings { EvalHasExtraBindings, EvalWithDefaultBindings };

::: js/src/vm/Interpreter.cpp
@@ +239,5 @@
>      JS_ASSERT(obj->getClass() == &js_NoSuchMethodClass);
>  
>      args.setCallee(obj->getSlot(JSSLOT_FOUND_FUNCTION));
>      args.setThis(vp[1]);
> +    args[0].set(obj->getSlot(JSSLOT_SAVED_ID));

Might as well make this getReservedSlot in passing, and same for two lines up.

::: js/src/vm/SelfHosting.cpp
@@ +472,5 @@
>      JS_ASSERT(args.length() == 3);
>      JS_ASSERT(args[0].isObject());
>      JS_ASSERT(args[1].isInt32());
>  
> +    args[0].toObject().setReservedSlot(args[1].get().toPrivateUint32(), args[2]);

Agreed about adding toPrivateUint32() instead of this file's changes.
Attachment #768062 - Flags: review?(jwalden+bmo) → review+
Attachment #768066 - Flags: review?(jwalden+bmo) → review+
Attachment #768069 - Flags: review?(jwalden+bmo) → review+
Attachment #768071 - Flags: review?(jwalden+bmo) → review+
You are correct: the changes needed for the browser to build are pretty minimal.
Attachment #768580 - Flags: review?(bzbarsky)
Comment on attachment 768580 [details] [diff] [review]
Part 5 - Browser and CTypes fixes

r=me on the changes, but these look to be on top of some other change to the jsfriendapi stuff that already removed the handleAt methods, and should land as part of the "remove handleAt" patch so we're not knowing pushing non-building changesets to the tree (because the more of those we have, the more bisecting sucks).
Attachment #768580 - Flags: review?(bzbarsky) → review+
Sorry, I should have mentioned that I plan to fold these 5 patches before pushing: they don't make much sense on their own, even despite totally busting the browser build.
What happened (or didn't happen) here?
Flags: needinfo?(terrence)
C++ default operator= happened here: weird, crashless behavior changes. I didn't have the spare couple of hours to track it down until yesterday. Should be landing as soon as I fix 3-weeks of merge bustage.
Flags: needinfo?(terrence)
https://hg.mozilla.org/mozilla-central/rev/1de22229a4f3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.