Last Comment Bug 770092 - JSScript::fullyInitFromEmitter() should only init JSScript
: JSScript::fullyInitFromEmitter() should only init JSScript
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: LazyBytecode
  Show dependency treegraph
 
Reported: 2012-07-02 00:00 PDT by Nicholas Nethercote [:njn]
Modified: 2012-07-06 07:48 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
(part 1) - Move the new script notification code out of JSScript::fullyInitFromEmitter(). (3.14 KB, patch)
2012-07-02 00:03 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
(part 2) - Don't set JSScript::function_ in JSScript::fullyInitFromEmitter(). (1.97 KB, patch)
2012-07-02 00:04 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
(part 3) - Move typeSetFunction() from JSScript to JSFunction. (4.13 KB, patch)
2012-07-02 00:05 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review
(part 4) - Move JSFunction initialization code out of JSScript::fullyInitFromEmitter(). (2.74 KB, patch)
2012-07-02 00:07 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-07-02 00:00:10 PDT
JSScript::fullyInitFromEmitter() sets fields in JSScript, unsurprisingly.  But it also sets some fields in the corresponding JSFunction, and calls the new script notifier functions.

The aim of this bug is to move the non-initialization work elsewhere,
because it is non-intuitive to have it in fullyInitFromEmitter(), and it
gets in the way of lazy bytecode generation.
Comment 1 Nicholas Nethercote [:njn] 2012-07-02 00:03:21 PDT
Created attachment 638274 [details] [diff] [review]
(part 1) - Move the new script notification code out of JSScript::fullyInitFromEmitter().

This patch moves the new script notification code out of
fullyInitFromEmitter().
Comment 2 Nicholas Nethercote [:njn] 2012-07-02 00:04:00 PDT
Created attachment 638275 [details] [diff] [review]
(part 2) - Don't set JSScript::function_ in JSScript::fullyInitFromEmitter().

This patch avoids setting JSScript::function_ in
JSScript::typeSetFunction(), which is a prerequisite for the next patch.
Comment 3 Nicholas Nethercote [:njn] 2012-07-02 00:05:28 PDT
Created attachment 638276 [details] [diff] [review]
(part 3) - Move typeSetFunction() from JSScript to JSFunction.

This patch moves JSScript::typeSetFunction() into JSFunction, which makes
sense, because it's now all about setting fields in JSFunction.  (I also
wonder if the function should be renamed?)
Comment 4 Nicholas Nethercote [:njn] 2012-07-02 00:07:13 PDT
Created attachment 638277 [details] [diff] [review]
(part 4) - Move JSFunction initialization code out of JSScript::fullyInitFromEmitter().

This patch moves the JSFunction initialization code out of
JSScript::fullyInitFromEmitter.
Comment 5 Brian Hackett (:bhackett) 2012-07-05 17:02:45 PDT
Comment on attachment 638276 [details] [diff] [review]
(part 3) - Move typeSetFunction() from JSScript to JSFunction.

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

::: js/src/jsfun.h
@@ +178,5 @@
>      inline void setExtendedSlot(size_t which, const js::Value &val);
>      inline const js::Value &getExtendedSlot(size_t which) const;
>  
> +    /* Constructs a new type for the function if necessary. */
> +    bool typeSetFunction(JSContext *cx, bool singleton = false);

Maybe setTypeForScriptedFunction?

::: js/src/jsinfer.cpp
@@ +5417,5 @@
>  
>  bool
> +JSFunction::typeSetFunction(JSContext *cx, bool singleton)
> +{
> +    JS_ASSERT(script());

This should also assert that script->function() == this

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