add a Evaluate() variant which can read a file

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 647439 [details] [diff] [review]
add and use in load()

CompileOptions is great!
Assignee: general → bpeterson
Attachment #647439 - Flags: review?(jimb)
(Assignee)

Comment 2

5 years ago
Created attachment 647441 [details] [diff] [review]
add and use in load()

Set filename correctly.
Attachment #647439 - Attachment is obsolete: true
Attachment #647439 - Flags: review?(jimb)
Attachment #647441 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #647441 - Flags: review? → review?(jimb)

Comment 3

5 years ago
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+
(Assignee)

Comment 4

5 years ago
Thanks for the review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/668a0ce3f6d7
https://hg.mozilla.org/mozilla-central/rev/668a0ce3f6d7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.