The default bug view has changed. See this FAQ.

JSScript::fullyInitFromEmitter() should only init JSScript

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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().
Attachment #638274 - Flags: review?(bhackett1024)
(Assignee)

Comment 2

5 years ago
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.
Attachment #638275 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
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?)
Attachment #638276 - Flags: review?(bhackett1024)
(Assignee)

Comment 4

5 years ago
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.
Attachment #638277 - Flags: review?(bhackett1024)
Attachment #638274 - Flags: review?(bhackett1024) → review+
Attachment #638275 - Flags: review?(bhackett1024) → review+
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
Attachment #638276 - Flags: review?(bhackett1024) → review+
Attachment #638277 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd4967738263
https://hg.mozilla.org/integration/mozilla-inbound/rev/3331772d38a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a88ef4eaf30
https://hg.mozilla.org/integration/mozilla-inbound/rev/f536e2d52eda
https://hg.mozilla.org/mozilla-central/rev/dd4967738263
https://hg.mozilla.org/mozilla-central/rev/3331772d38a8
https://hg.mozilla.org/mozilla-central/rev/2a88ef4eaf30
https://hg.mozilla.org/mozilla-central/rev/f536e2d52eda
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.