Closed Bug 1100579 Opened 10 years ago Closed 10 years ago

Use the passed-in JS::CompileOptions, not overloading, to decide whether we want an rval in JS::Evaluate and company

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files)

      No description provided.
Depends on: 1100580
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8524255 - Flags: review?(jwalden+bmo)
Comment on attachment 8524253 [details] [diff] [review]
part 1.  Remove the overloads of JS::Evaluate that don't take an rval mutable handle, and control the behavior via the JS::CompileOptions instead

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

r=me on the Gecko bits.
Attachment #8524253 - Flags: review?(bobbyholley) → review+
Comment on attachment 8524253 [details] [diff] [review]
part 1.  Remove the overloads of JS::Evaluate that don't take an rval mutable handle, and control the behavior via the JS::CompileOptions instead

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

::: js/src/jsapi-tests/testSourcePolicy.cpp
@@ +23,5 @@
>      JS::RootedFunction fun(cx);
>      JS::AutoObjectVector emptyScopeChain(cx);
> +    // But when compiling a function we don't want to use no-rval
> +    // mode, since it's not supported for functions.
> +    opts.setNoScriptRval(false);

This derives from MOZ_ASSERT(!bce->script->noScriptRval()); in JSScript::fullyInitFromEmitter, correct?

::: js/src/jsapi.cpp
@@ +4825,5 @@
>    SourceBufferHolder srcBuf(chars, length, SourceBufferHolder::NoOwnership);
>    return ::Evaluate(cx, obj, optionsArg, srcBuf, rval);
>  }
>  
> +extern JS_PUBLIC_API(bool)

Mild preference for no extern on this, and seeing it only on the declaration.
Attachment #8524253 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8524255 [details] [diff] [review]
part 2.  Kill off JS_Evaluate(UC)Script

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

::: js/src/jsapi-tests/testJSEvaluateScript.cpp
@@ +14,5 @@
>      static const char src[] = "var x = 5;";
>  
>      JS::RootedValue retval(cx);
> +    JS::CompileOptions opts(cx);
> +    CHECK(JS::Evaluate(cx, obj, opts.setFileAndLine(__FILE__, __LINE__),

Bleh, I wish we could do this without the setFAL call in the method-call.

::: js/src/jsapi.h
@@ +4030,1 @@
>   * for each context in the application, if you pass parented objects as the obj

Just above this line is a claim that embedders should ContextOptionsRef(cx).setVarObjFix(true).  Please fix this to RuntimeOptionsRef(rt).setVarObjFix(true).

@@ +4030,5 @@
>   * for each context in the application, if you pass parented objects as the obj
>   * parameter, or may ever pass such objects in the future.
>   *
>   * Why a runtime option?  The alternative is to add six or so new API entry
>   * points with signatures matching the following six, and that doesn't seem

"six" is out of date.  Maybe "...to add APIs duplicating those below, for the other varobjfix sense, and that doesn't seem..." or so.

@@ +4039,1 @@
>   * etc., entry points.

"The RuntimeOptionsRef adjustment, OTOH, ..."

Should rewrap the etc. bit while you're changing here.

::: js/src/shell/js.cpp
@@ +2625,5 @@
>              JS_ReportError(cx, "Invalid scope argument to evalcx");
>              return false;
>          }
> +        JS::CompileOptions opts(cx);
> +        opts.setFileAndLine(filename.get(), lineno);

Hmm.  It'd be nice if we could make this take a Move(filename) or something, but I guess the signatures are all off.  Never mind for now.
Attachment #8524255 - Flags: review?(jwalden+bmo) → review+
> This derives from MOZ_ASSERT(!bce->script->noScriptRval()); in 
> JSScript::fullyInitFromEmitter, correct?

Actually, from the same assert in EmitStatement in the bce->sc->isFunctionBox() case.  But yes, same idea.

> Mild preference for no extern on this, and seeing it only on the declaration.

Will do.

> Bleh, I wish we could do this without the setFAL call in the method-call.

We can if we're willing to have our line be off-by-one... I wasn't sure which would be better and went with preserving behavior.

> Please fix this to RuntimeOptionsRef(rt).setVarObjFix(true).

Will do.

> Maybe "...to add APIs duplicating those below, for the other varobjfix sense, and that
> doesn't seem..." or so.

Will do.

> Should rewrap the etc. bit while you're changing here.

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