Closed Bug 779038 Opened 9 years ago Closed 9 years ago

add a Evaluate() variant which can read a file


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Benjamin, Assigned: Benjamin)



(1 file, 1 obsolete file)

No description provided.
Attached patch add and use in load() (obsolete) — Splinter Review
CompileOptions is great!
Assignee: general → bpeterson
Attachment #647439 - Flags: review?(jimb)
Set filename correctly.
Attachment #647439 - Attachment is obsolete: true
Attachment #647439 - Flags: review?(jimb)
Attachment #647441 - Flags: review?
Attachment #647441 - Flags: review? → review?(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)?
Attachment #647441 - Flags: review?(jimb) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.