Treehydra: Add js mode setting function. strict + version

RESOLVED FIXED

Status

Firefox Build System
Source Code Analysis
RESOLVED FIXED
11 years ago
5 months ago

People

(Reporter: (dormant account), Assigned: dmandelin)

Tracking

Trunk
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
I know spidermonkey uses the version() function to query and specify version. I don't like the idea of having a function per flag. I propose setting these flags as require ({version:"1.7", strict:true}). This way other flags can be easily added later.
(Reporter)

Updated

11 years ago
Blocks: 423898
That kind of API makes for nice, readable call sites.

I wanted to point out the options cmd in case you missed it:

js> help("options")
JavaScript-C 1.8.0 pre-release 1 2007-10-03
Command                  Description
=======                  ===========
options([option ...])    Get or toggle JavaScript options
js> options()

js> options("strict")         

js> options()
strict

/be
(Assignee)

Updated

10 years ago
Assignee: nobody → dmandelin
(Assignee)

Comment 2

10 years ago
Created attachment 312404 [details] [diff] [review]
Proposed patch

I followed the original spec, except that I also added support for JSOPTION_WERROR using { werror: true }. Couple of questions about this:

1. It would definitely be easier to implement the argument decoding in JS instead of C+JSAPI. I went with the C version because (a) the spec seemed to suggest Taras wanted fewer entry points into C (and you get the max benefit of decoding in JS with multiple entry points) and (b) I don't think there's a script that gets loaded with both Dehydra and Treehydra, so for now there's no place to put JS code like that. 

2. The semantics of the 'require' function are complex enough to merit thorough testing. I've done some, but it seems it's time for more of a testing framework, and I'm wondering what thoughts people have on that issue. Some sort of unit test system in JS, like the thing I hacked up for bug 425569? And where should test cases live in the Dehydra source tree?
(Reporter)

Updated

10 years ago
Depends on: 425845
(Reporter)

Comment 3

10 years ago
(In reply to comment #2)
> Created an attachment (id=312404) [details]
> Proposed patch
> 
> I followed the original spec, except that I also added support for
> JSOPTION_WERROR using { werror: true }. Couple of questions about this:

Cool.

> 
> 1. It would definitely be easier to implement the argument decoding in JS
> instead of C+JSAPI. I went with the C version because (a) the spec seemed to
> suggest Taras wanted fewer entry points into C (and you get the max benefit of
> decoding in JS with multiple entry points) and (b) I don't think there's a
> script that gets loaded with both Dehydra and Treehydra, so for now there's no
> place to put JS code like that. 

system.js should be loaded by both.

> 
> 2. The semantics of the 'require' function are complex enough to merit thorough
> testing. I've done some, but it seems it's time for more of a testing
> framework, and I'm wondering what thoughts people have on that issue. Some sort
> of unit test system in JS, like the thing I hacked up for bug 425569? And where
> should test cases live in the Dehydra source tree?
> 

I haven't looked at your test stuff yet, but yes and yes. A testing framework has been a secret todo of mine. I just keep adding tests to the test/Makefile, but feel free to propose something better.
(Reporter)

Comment 4

10 years ago
nevermind, my recent changes made system.js dehydra-specific. We can make a common file if you want to use. I encourage JS over C :)
(Assignee)

Comment 5

10 years ago
(In reply to comment #4)
> nevermind, my recent changes made system.js dehydra-specific. We can make a
> common file if you want to use. I encourage JS over C :)

After thinking about it a while, I'm fairly convinced there isn't much benefit to implementing part of 'require' in JS unless the JS part can pick apart all the arguments and call very lightweight separate C functions 'require_option' and 'require_strict'. Otherwise, the complexity of the C part decreases very little, but system complexity goes up with the new JS piece, which also has to be kept in sync with the C part. So I kind of favor leaving it the way it is, implemented in C. But if you have some specific ideas for doing it better, let me know.

In the meantime I'll make up a better set of tests.
(Assignee)

Updated

10 years ago
Summary: Add js mode setting function. strict + version → Treehydra: Add js mode setting function. strict + version
(Assignee)

Comment 6

10 years ago
Created attachment 312800 [details] [diff] [review]
Unit tests

Requires the unit test framework on bug 425794.
(Reporter)

Comment 7

10 years ago
Please set the depends/blocks stuff when needed. 
Depends on: 425794
(Assignee)

Comment 8

10 years ago
(In reply to comment #7)
> Please set the depends/blocks stuff when needed. 
> 

Shouldn't this block bug 425845 rather than the other way around? You can still run with strict off, and it's hard to test 425845 without this one.
(Reporter)

Comment 9

10 years ago
Comment on attachment 312404 [details] [diff] [review]
Proposed patch

>diff -r 1ae10a7fafd8 dehydra.c
>--- a/dehydra.c	Fri Mar 28 15:10:08 2008 -0700
>+++ b/dehydra.c	Fri Mar 28 16:26:42 2008 -0700
>@@ -71,7 +71,8 @@ void dehydra_init(Dehydra *this, const c
>     {"read_file",       ReadFile,       1},
>     {"error",           Error,          0},
>     {"warning",         Warning,        0},
>-    {"version",         Version,        0},
>+    {"get_version",     GetVersion,     0},

No need for version if require returns defaults

>+    {"require",         Require,        0},
> #ifdef JS_GC_ZEAL
>     JS_FN("gczeal",     GCZeal,     1,1,0),
> #endif
>diff -r 1ae10a7fafd8 dehydra_builtins.c
>--- a/dehydra_builtins.c	Fri Mar 28 15:10:08 2008 -0700
>+++ b/dehydra_builtins.c	Fri Mar 28 16:26:42 2008 -0700
>@@ -14,6 +14,82 @@
> #include "dehydra.h"
> #include "dehydra_builtins.h"
> #include "xassert.h"
>+
>+JSBool require_version(JSContext *cx, jsval val) {
>+  JSString *version_str = JS_ValueToString(cx, val);
>+  if (!version_str) return JS_FALSE;
>+  JS_AddRoot(cx, &version_str);
>+  char *version_cstr = JS_GetStringBytes(version_str);
>+  JSVersion version = JS_StringToVersion(version_cstr);
>+  JSBool retval;
>+  if (version == JSVERSION_UNKNOWN) {
>+    JS_ReportError(cx, "Invalid version '%s'", version_cstr);
>+    retval = JS_FALSE;
>+  } else {
>+    JS_SetVersion(cx, version);
>+    retval = JS_TRUE;
>+  }
>+  JS_RemoveRoot(cx, &version_str);
>+  return retval;
>+}

Might as well pass the Dehydra*this instead of cx, but that's mostly a style issue, up to you.
No need to root things, since their are transitively rooted since they are properties of an object passed as an argument.


>+
>+JSBool require_option(JSContext *cx, jsval val, uint32 option) {
>+  JSBool flag;
>+  if (!JS_ValueToBoolean(cx, val, &flag)) return JS_FALSE;
>+  if (flag) {
>+    JS_SetOptions(cx, JS_GetOptions(cx) | option);
>+  } else {
>+    JS_SetOptions(cx, JS_GetOptions(cx) & ~option);
>+  }
>+  return JS_TRUE;
>+}
>+
>+JSBool dispatch_require(JSContext *cx, const char *prop_name, jsval prop_val) {
>+  if (strcmp(prop_name, "version") == 0) {
>+    return require_version(cx, prop_val);
>+  } else if (strcmp(prop_name, "strict") == 0) {
>+    return require_option(cx, prop_val, JSOPTION_STRICT);
>+  } else if (strcmp(prop_name, "werror") == 0) {
>+    return require_option(cx, prop_val, JSOPTION_WERROR);
>+  } else {
>+    JS_ReportWarning(cx, "Unrecognized require keyword '%s'", prop_name);
>+    return JS_TRUE;
>+  }
>+}

!strcmp, is a fairly idiomatic thing in C when testing for comparison.

>+
>+JSBool Require(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>+               jsval *rval)
>+{
>+  JSObject *args;
>+  if (!JS_ConvertArguments(cx, argc, argv, "o", &args)) return JS_FALSE;
>+  JSIdArray *prop_ids = JS_Enumerate(cx, args);
>+  if (!prop_ids) return JS_FALSE;
>+
>+  JSBool retval = JS_TRUE;
>+  int i;
>+  for (i = 0; i < prop_ids->length; ++i) {
>+    jsval prop;
>+    JSBool rv = JS_IdToValue(cx, prop_ids->vector[i], &prop);
>+    xassert(rv);
>+    JS_AddRoot(cx, &prop);
>+    JSString *prop_str = JSVAL_TO_STRING(prop);
>+    char *prop_name = JS_GetStringBytes(prop_str);
>+    jsval prop_val;
>+    rv = JS_GetProperty(cx, args, prop_name, &prop_val);
>+    xassert(rv);
>+    JS_AddRoot(cx, &prop_val);
>+
>+    rv = dispatch_require(cx, prop_name, prop_val);
>+    if (rv == JS_FALSE) retval = JS_FALSE;
>+
>+    JS_RemoveRoot(cx, &prop_val);
>+    JS_RemoveRoot(cx, &prop_name);
>+  }
>+  
>+  JS_DestroyIdArray(cx, prop_ids);
>+
>+  return retval;
>+}

Once again. No rooting is needed here.
One of my goals is for require is to be self-documenting. Calling require should return an object with all of the current values. 

> 
> JSBool Print(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>              jsval *rval)
>@@ -64,13 +140,16 @@ JSBool Warning(JSContext *cx, JSObject *
> }
> 
> 
>-JSBool Version(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>+JSBool GetVersion(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>                jsval *rval)
> {
>-  if (argc > 0 && JSVAL_IS_INT(argv[0]))
>-    *rval = INT_TO_JSVAL(JS_SetVersion(cx, (JSVersion) JSVAL_TO_INT(argv[0])));
>-  else
>-    *rval = INT_TO_JSVAL(JS_GetVersion(cx));
>+  const char *version_cstr = JS_VersionToString(JS_GetVersion(cx));
>+  if (version_cstr == NULL) {
>+    *rval = JSVAL_VOID;
>+    return JS_TRUE;
>+  }
>+  JSString *version_str = JS_NewStringCopyZ(cx, version_cstr);
>+  *rval = STRING_TO_JSVAL(version_str);
>   return JS_TRUE;
> }

Should get rid of version().
> 
>diff -r 1ae10a7fafd8 dehydra_builtins.h
>--- a/dehydra_builtins.h	Fri Mar 28 15:10:08 2008 -0700
>+++ b/dehydra_builtins.h	Fri Mar 28 16:26:42 2008 -0700
>@@ -3,32 +3,22 @@
> 
> /* JS Natives */
> 
>-JSBool Print(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>-             jsval *rval);
>+#define DH_JSNATIVE(fname) JSBool fname(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval);
> 
>-JSBool Error(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>-             jsval *rval);
>+DH_JSNATIVE(Require);
>+DH_JSNATIVE(GetVersion);
>+DH_JSNATIVE(Include);
> 
>+DH_JSNATIVE(Error);
>+DH_JSNATIVE(Print);
>+DH_JSNATIVE(Warning);
> 
>-JSBool Warning(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>-               jsval *rval);
>+DH_JSNATIVE(WriteFile);
>+DH_JSNATIVE(ReadFile);
> 
>-JSBool Version(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
>-               jsval *rval);
>+DH_JSNATIVE(obj_hashcode);
> 
> void ReportError(JSContext *cx, const char *message, JSErrorReport *report);
>-
>-JSBool WriteFile (JSContext *cx, JSObject *obj, uintN argc,
>-                  jsval *argv, jsval *rval);
>-
>-JSBool ReadFile(JSContext *cx, JSObject *obj, uintN argc,
>-                jsval *argv, jsval *rval);
>-
>-JSBool Include(JSContext *cx, JSObject *obj, uintN argc,
>-               jsval *argv, jsval *rval);
>-
>-JSBool obj_hashcode(JSContext *cx, JSObject *obj, uintN argc,
>-                    jsval *argv, jsval *rval);
> 
> /* Related C functions */
> 

I approve of the preprocessor usage here.
Attachment #312404 - Flags: review-
(Reporter)

Updated

10 years ago
Blocks: 425846
(Assignee)

Comment 10

10 years ago
(In reply to comment #9)
> >-    {"version",         Version,        0},
> >+    {"get_version",     GetVersion,     0},
> 
> No need for version if require returns defaults

You mean you want it to return the option values that were set just before the call, I assume?

> >+JSBool require_version(JSContext *cx, jsval val) {

> Might as well pass the Dehydra*this instead of cx, but that's mostly a style
> issue, up to you.

I passed the cx because that's what Require gets and this is passed from require.

Another thing is that it bothers me to be passing around a "Dehydra" object in Treehydra. I've been meaning to ask you about that: it would make sense to me to pull the common core into non-tool-specific code, but I wasn't sure if it was worth the effort at this time.

> No need to root things, since their are transitively rooted since they are
> properties of an object passed as an argument. 

I'm pretty new to rooting, but I'm not sure about that. I think ValueToString performs a conversion if the input was not a string. Wouldn't that create a new string which would need to be rooted?

> !strcmp, is a fairly idiomatic thing in C when testing for comparison.

I can change it. (I'm in the habit of using '== 0' as something that jumps out at my eyes--those leading !s are easy to miss, esp next to (s.)
 
> >+JSBool Require(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
> >+               jsval *rval)
> >+{
> >+  JSObject *args;
> >+  if (!JS_ConvertArguments(cx, argc, argv, "o", &args)) return JS_FALSE;
> >+  JSIdArray *prop_ids = JS_Enumerate(cx, args);
> >+  if (!prop_ids) return JS_FALSE;
> >+
> >+  JSBool retval = JS_TRUE;
> >+  int i;
> >+  for (i = 0; i < prop_ids->length; ++i) {
> >+    jsval prop;
> >+    JSBool rv = JS_IdToValue(cx, prop_ids->vector[i], &prop);
> >+    xassert(rv);
> >+    JS_AddRoot(cx, &prop);

> Once again. No rooting is needed here.

This time, I'm pretty sure you are right about the second AddRoot, but I still wonder about the first one. The whole jsid thing is very mysterious to me, so I didn't know if the jsvals you get out of it are existing rooted objects or not. I suppose it wouldn't make much sense for them to be new objects.

> One of my goals is for require is to be self-documenting. Calling require
> should return an object with all of the current values. 
>
> Should get rid of version().

OK.

> I approve of the preprocessor usage here.

Yay!
(Reporter)

Comment 11

10 years ago
(In reply to comment #10)
> (In reply to comment #9)
> > >-    {"version",         Version,        0},
> > >+    {"get_version",     GetVersion,     0},
> > 
> > No need for version if require returns defaults
> 
> You mean you want it to return the option values that were set just before the
> call, I assume?

Options that are current. 
foo = require(version:666} will return {version:666, strict:false}...
foo = require() will return same as above

> 
> > >+JSBool require_version(JSContext *cx, jsval val) {
> 
> > Might as well pass the Dehydra*this instead of cx, but that's mostly a style
> > issue, up to you.
> 
> I passed the cx because that's what Require gets and this is passed from
> require.
> 
> Another thing is that it bothers me to be passing around a "Dehydra" object in
> Treehydra. I've been meaning to ask you about that: it would make sense to me
> to pull the common core into non-tool-specific code, but I wasn't sure if it
> was worth the effort at this time.

Yes, refactorings to improve style are always welcome.
I try to keep code clean and in sensible files, but that tends to get out of hand. So file bugs with patches or just file and I'll refactor myself. 
Dehydra* isn't specific to dehydra really. Do we need a new name for that?

> 
> > No need to root things, since their are transitively rooted since they are
> > properties of an object passed as an argument. 
> 
> I'm pretty new to rooting, but I'm not sure about that. I think ValueToString
> performs a conversion if the input was not a string. Wouldn't that create a new
> string which would need to be rooted?

yes and no. It's a good idea to keep things rooted, but in this case you don't make any calls to js functions that might do allocations(afaik) so it's safe.
This reminds me: need to move gczeal to require.


> 
> > !strcmp, is a fairly idiomatic thing in C when testing for comparison.
> 
> I can change it. (I'm in the habit of using '== 0' as something that jumps out
> at my eyes--those leading !s are easy to miss, esp next to (s.)

Do as you please.

> 
> > >+JSBool Require(JSContext *cx, JSObject *obj, uintN argc, jsval *argv,
> > >+               jsval *rval)
> > >+{
> > >+  JSObject *args;
> > >+  if (!JS_ConvertArguments(cx, argc, argv, "o", &args)) return JS_FALSE;
> > >+  JSIdArray *prop_ids = JS_Enumerate(cx, args);
> > >+  if (!prop_ids) return JS_FALSE;
> > >+
> > >+  JSBool retval = JS_TRUE;
> > >+  int i;
> > >+  for (i = 0; i < prop_ids->length; ++i) {
> > >+    jsval prop;
> > >+    JSBool rv = JS_IdToValue(cx, prop_ids->vector[i], &prop);
> > >+    xassert(rv);
> > >+    JS_AddRoot(cx, &prop);
> 
> > Once again. No rooting is needed here.
> 
> This time, I'm pretty sure you are right about the second AddRoot, but I still
> wonder about the first one. The whole jsid thing is very mysterious to me, so I
> didn't know if the jsvals you get out of it are existing rooted objects or not.
> I suppose it wouldn't make much sense for them to be new objects.

They aren't new objects afaik. 
You are right, need to root in this case because of conversions later on.
(Assignee)

Comment 12

10 years ago
Created attachment 313247 [details] [diff] [review]
New patch

- get_version builtin function is gone.

- require returns the current option values (after applying any args)

- removed unneeded AddRoots, replaced the other one with the argv[argc+1] idiom. [1]

- updated unit test cases to go with new spec.

[1] http://developer.mozilla.org/en/docs/SpiderMonkey_Garbage_Collection_Tips
Attachment #312404 - Attachment is obsolete: true
Attachment #312800 - Attachment is obsolete: true
Attachment #313247 - Flags: review?(tglek)
(Assignee)

Updated

10 years ago
Blocks: 426669
(Reporter)

Comment 13

10 years ago
Comment on attachment 313247 [details] [diff] [review]
New patch

Looks good
Attachment #313247 - Flags: review?(tglek) → review+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
No longer depends on: 425845

Updated

5 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.