Last Comment Bug 779038 - add a Evaluate() variant which can read a file
: add a Evaluate() variant which can read a file
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla17
Assigned To: :Benjamin Peterson
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-07-30 22:19 PDT by :Benjamin Peterson
Modified: 2012-08-01 02:52 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

add and use in load() (7.90 KB, patch)
2012-07-30 22:20 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
add and use in load() (7.97 KB, patch)
2012-07-30 22:25 PDT, :Benjamin Peterson
jimb: review+
Details | Diff | Splinter Review

Description User image :Benjamin Peterson 2012-07-30 22:19:46 PDT

Comment 1 User image :Benjamin Peterson 2012-07-30 22:20:42 PDT
Created attachment 647439 [details] [diff] [review]
add and use in load()

CompileOptions is great!
Comment 2 User image :Benjamin Peterson 2012-07-30 22:25:34 PDT
Created attachment 647441 [details] [diff] [review]
add and use in load()

Set filename correctly.
Comment 3 User image Jim Blandy :jimb 2012-07-31 14:55:33 PDT
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 User image :Benjamin Peterson 2012-07-31 15:27:46 PDT
Thanks for the review.
Comment 5 User image Ed Morley [:emorley] 2012-08-01 02:52:54 PDT

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