Last Comment Bug 771705 - THE NUMBER OF JS_COMPILE* FUNCTIONS IS TOO DAMN HIGH
: THE NUMBER OF JS_COMPILE* FUNCTIONS IS TOO DAMN HIGH
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jim Blandy :jimb
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 16:10 PDT by Jim Blandy :jimb
Modified: 2012-08-03 06:51 PDT (History)
8 users (show)
jimb: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Pull out compilation variants into a CompileOptions structure. (60.81 KB, patch)
2012-07-07 12:15 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Pull out compilation variants into a CompileOptions structure. (60.94 KB, patch)
2012-07-14 21:29 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Pull out compilation variants into a CompileOptions structure. (61.28 KB, patch)
2012-07-16 13:24 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Pull out compilation variants into a CompileOptions structure. (60.71 KB, patch)
2012-07-18 12:23 PDT, Jim Blandy :jimb
luke: review+
Details | Diff | Splinter Review

Description Jim Blandy :jimb 2012-07-06 16:10:20 PDT
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] 2012-07-07 04:33:02 PDT
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 2012-07-07 12:15:21 PDT
Created attachment 639985 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Here's something that compiles...
Comment 3 David Mandelin [:dmandelin] 2012-07-09 16:02:15 PDT
Looks good.
Comment 4 Jim Blandy :jimb 2012-07-14 21:29:31 PDT
Created attachment 642335 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Rebased.
Comment 5 Jim Blandy :jimb 2012-07-14 21:30:13 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=cdecb369e786
Comment 6 Jim Blandy :jimb 2012-07-16 13:24:02 PDT
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 2012-07-16 13:24:37 PDT
Created attachment 642698 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Use of AutoVersionAPI updated.
Comment 8 Jim Blandy :jimb 2012-07-17 14:20:14 PDT
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 2012-07-18 12:21:28 PDT
All looks good. Full try:
https://tbpl.mozilla.org/?tree=Try&rev=5020615a1861
Comment 10 Jim Blandy :jimb 2012-07-18 12:23:06 PDT
Created attachment 643497 [details] [diff] [review]
Pull out compilation variants into a CompileOptions structure.

Updated patch.
Comment 11 Luke Wagner [:luke] 2012-07-19 09:09:56 PDT
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 2012-07-19 16:56:49 PDT
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 2012-07-23 15:24:51 PDT
ONE MORE TIME: https://tbpl.mozilla.org/?tree=Try&rev=948fba659d35
Comment 14 Jim Blandy :jimb 2012-07-25 17:29:33 PDT
That looks okay.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7fa061e61a
Comment 15 Ed Morley [:emorley] 2012-07-26 05:08:36 PDT
https://hg.mozilla.org/mozilla-central/rev/6e7fa061e61a
Comment 16 Jason Orendorff [:jorendorff] 2012-08-03 06:51:02 PDT
Huzzah!

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