The default bug view has changed. See this FAQ.

THE NUMBER OF JS_COMPILE* FUNCTIONS IS TOO DAMN HIGH

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
At present, jsapi.h declares:

JS_CompileScript
JS_CompileScriptForPrincipals
JS_CompileScriptForPrincipalsVersion
JS_CompileUCScript
JS_CompileUCScriptForPrincipals
JS_CompileUCScriptForPrincipalsVersion
JS_CompileUCScriptForPrincipalsVersionOrigin
JS_CompileUTF8File
JS_CompileUTF8FileHandle
JS_CompileUTF8FileHandleForPrincipals
JS_CompileUTF8FileHandleForPrincipalsVersion

However, bug 765993 would like to provide source map information, and bug 637572 would like to provide origin information. The current pattern is clearly not what we want to follow.

Instead, we should have a JS::CompileOptions type, with member functions for setting various options (and checking for consistency). Then, we should have a JS::Compile function, overloaded for each way of passing text:
- char *, length
- jschar *, length
- FILE *

This would reduce the length of many argument lists within SM itself, as well.
Yes please!

For bug 462300, I'll most likely want to add yet another set of these functions in the current scheme of things, making this change even more desirable.
(Assignee)

Comment 2

5 years ago
Created attachment 639985 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Here's something that compiles...
Assignee: general → jimb
Status: NEW → ASSIGNED
Looks good.
Whiteboard: [js:t]
(Assignee)

Comment 4

5 years ago
Created attachment 642335 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Rebased.
Attachment #639985 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=cdecb369e786
(Assignee)

Comment 6

5 years ago
I screwed up the version handling; turns out that AutoVersionAPI *modifies* the version passed to it.

New try: https://tbpl.mozilla.org/?tree=Try&rev=69ff744bd1f4
(Assignee)

Comment 7

5 years ago
Created attachment 642698 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Use of AutoVersionAPI updated.
(Assignee)

Updated

5 years ago
Attachment #642335 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Third try push: I mis-read the code that reads a file into memory, assuming that 'len' was the length. Shoot me now. Rewritten using Vector<char, ...>.

https://tbpl.mozilla.org/?tree=Try&rev=3d2987f423fa
(Assignee)

Comment 9

5 years ago
All looks good. Full try:
https://tbpl.mozilla.org/?tree=Try&rev=5020615a1861
(Assignee)

Updated

5 years ago
Attachment #642698 - Flags: review?(luke)
(Assignee)

Updated

5 years ago
Attachment #642698 - Flags: review?(luke)
(Assignee)

Comment 10

5 years ago
Created attachment 643497 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Updated patch.
Attachment #642698 - Attachment is obsolete: true
Attachment #643497 - Flags: review?(luke)

Comment 11

5 years ago
Comment on attachment 643497 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

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

Looks good to me, although you might want to solicit njn's opinion since he has been touching the same area recently.

::: js/src/jsapi.h
@@ +4937,5 @@
> +Compile(JSContext *cx, JSHandleObject obj, const CompileOptions &options,
> +        const char *bytes, size_t length);
> +
> +extern JS_PUBLIC_API(JSScript *)
> +Compile(JSContext *cx, JSHandleObject obj, CompileOptions options,

This API passes CompileOptions by value while most of the others use const &.  In general, I doubt perf here matters enough to warrant const & over passing by value (we're about to parse, after all), so I'd be just as happy if you converted all the other APIs to pass by value.  By value also is a bit shorter to read :)  Whichever you choose, I see a few other const&/value inconsistencies (e.g., below), so it'd be good to give the patch a quick scan to make sure they are all one way or the other.
Attachment #643497 - Flags: review?(luke) → review+
(Assignee)

Comment 12

5 years ago
On IRC it was resolved that both George Sand and T.S. Eliot would prefer that CompileOptions be consistently passed by value.
(Assignee)

Comment 13

5 years ago
ONE MORE TIME: https://tbpl.mozilla.org/?tree=Try&rev=948fba659d35
(Assignee)

Comment 14

5 years ago
That looks okay.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7fa061e61a
Flags: in-testsuite-
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/6e7fa061e61a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Huzzah!
You need to log in before you can comment on or make changes to this bug.