771705 - THE NUMBER OF JS_COMPILE* FUNCTIONS IS TOO DAMN HIGH

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Till Schneidereit [:till]]

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.

Comment 2

[Jim Blandy :jimb]

Attachment: Pull out compilation variants into a CompileOptions structure.

Here's something that compiles...

Comment 3

[David Mandelin [:dmandelin]]

Looks good.

Comment 4

[Jim Blandy :jimb]

Attachment: Pull out compilation variants into a CompileOptions structure.

Rebased.

Comment 5

[Jim Blandy :jimb]

Try: https://tbpl.mozilla.org/?tree=Try&rev=cdecb369e786

Comment 6

[Jim Blandy :jimb]

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

Comment 7

[Jim Blandy :jimb]

Attachment: Pull out compilation variants into a CompileOptions structure.

Use of AutoVersionAPI updated.

Comment 8

[Jim Blandy :jimb]

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

Comment 9

[Jim Blandy :jimb]

All looks good. Full try:
https://tbpl.mozilla.org/?tree=Try&rev=5020615a1861

Comment 10

[Jim Blandy :jimb]

Attachment: Pull out compilation variants into a CompileOptions structure.

Updated patch.

Comment 11

[Luke Wagner [:luke]]

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.

Comment 12

[Jim Blandy :jimb]

On IRC it was resolved that both George Sand and T.S. Eliot would prefer that CompileOptions be consistently passed by value.

Comment 13

[Jim Blandy :jimb]

ONE MORE TIME: https://tbpl.mozilla.org/?tree=Try&rev=948fba659d35

Comment 14

[Jim Blandy :jimb]

That looks okay.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7fa061e61a

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6e7fa061e61a

Comment 16

[Jason Orendorff [:jorendorff]]

Huzzah!