779038 - add a Evaluate() variant which can read a file

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[:Benjamin Peterson]

Attachment: add and use in load()

CompileOptions is great!

Comment 2

[:Benjamin Peterson]

Attachment: add and use in load()

Set filename correctly.

Comment 3

[Jim Blandy :jimb]

Comment on attachment 647441 [details] [diff] [review]
add and use in load()

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

Looks great --- thanks! r=me, with the UTF8 thing fixed. Please consider the "Auto" class and the JS::Compile filename fix as well.

::: js/src/jsapi.cpp
@@ +5058,5 @@
>      return true;
>  }
>  
> +static FILE *
> +OpenFile(JSContext *cx, const char *filename)

You need a comment atop this function: a one-line summary of what it does, and then explaining that "-" is special, and that it might return stdin, which mustn't be closed.

You know what would be cool? A class that has a fallible "open" member function, and a destructor that fcloses (or does not fclose) the FILE * as appropriate. Just a suggestion; doesn't need to be in this bug unless you feel like it. We usually give such classes a name beginning with "Auto".

@@ +5646,5 @@
> +        fclose(fp);
> +    if (!ok)
> +        return NULL;
> +
> +    options = options.setUTF8(true).setFileAndLine(filename, 1);

The JS:: functions don't assume UTF8. It should be up to the caller to set that if they want it.

It's also the case that JS::Compile(..., filename) doesn't set options.filename or options.lineno, but I think that's a bug. JS::Compile(..., filename) and JS::Evaluate(..., filename) should set those in options. Could you fix JS::Compile(..., filename)?

Comment 4

[:Benjamin Peterson]

Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/668a0ce3f6d7

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/668a0ce3f6d7