Dave Herman [:dherman] Assignee
	Description • 14 years ago

Generating source location information allocates a fair amount of data per node, and also uglifies the object when debugging. Add an optional flag to disable source location information.

In the process, merge the existing optional parameters into a configuration object, so that Reflect.parse always takes either 1 or 2 arguments, the second being the configuration object.

Dave

	Jim Blandy :jimb
	Comment 2 • 14 years ago

Comment on attachment 473937 [details] [diff] [review]
configuration object instead of optional args; src loc is optional

+    if (prop)
+        obj2->dropProperty(cx, prop);
+
+    if (prop == NULL) {
+        *foundp = false;
+        return true;
+    }
+
+    *foundp = true;
+    return js_GetProperty(cx, obj2, id, vp);

Just a thought: how would this look combined into 'then' and 'else' clauses
in a single 'if (prop)'?

+        if (!arg.isObject()) {
+            JSString *str = js_ValueToString(cx, arg);
+            const char *chars = js_GetStringBytes(NULL, str);
+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_UNEXPECTED_TYPE,
+                                 chars, "not an object");
             return JS_FALSE;

I think we generally use js_ReportValueErrorFlags here, so the expression
appears in the error message.

-        filename = js_GetStringBytes(NULL, str);
+        }
+
+        JSObject *config = arg.toObjectOrNull();
+        JS_ASSERT(config);

This JS_ASSERT seems gratuitous, since we just called isObject a few lines
above.

+
+        Value prop;
+        bool found;
+
+        /* config.loc */
+        if (!GetProperty(cx, config, "loc", &prop, &found))
+            return JS_FALSE;
+
+        if (found)
+            loc = js_ValueToBoolean(prop);
+
+        if (loc) {
+            /* config.source */
+            if (!GetProperty(cx, config, "source", &prop, &found))
+                return JS_FALSE;
+
+            if (found) {
+                JSString *str = js_ValueToString(cx, prop);
+                if (!str)
+                    return JS_FALSE;
+                filename = js_GetStringBytes(NULL, str);

I didn't notice this in my earlier review: the memory returned by
js_GetStringBytes only lives as long as the string itself does. Unless you
know that the configuration object is going to remain alive until you
return, it might be nicer to call js_DeflateString(cx, str->chars(),
str->length()) to get your own copy.

(I don't think the conservative GC notices and preserves the strings
returned by js_GetStringBytes. And ASTSerializer shouldn't depend on being
allocated on the stack anyway.)

-var withFile = Reflect.parse("42", "foo.js");
-var withFileAndLine = Reflect.parse("42", "foo.js", 111);
+var withFile = Reflect.parse("42", {source:"foo.js"});
+var withFileAndLine = Reflect.parse("42", {source:"foo.js", line:111});

How does this work?  You haven't set a 'loc' property, so shouldn't the other properties of the config object be ignored?

	Brendan Eich [:brendan]
	Comment 3 • 14 years ago

(In reply to comment #2)
> Comment on attachment 473937 [details] [diff] [review]
> configuration object instead of optional args; src loc is optional
> 
> +    if (prop)
> +        obj2->dropProperty(cx, prop);
> +
> +    if (prop == NULL) {
> +        *foundp = false;
> +        return true;
> +    }
> +
> +    *foundp = true;
> +    return js_GetProperty(cx, obj2, id, vp);
> 
> Just a thought: how would this look combined into 'then' and 'else' clauses
> in a single 'if (prop)'?

Rather, put the if (!prop) {...} block first (nit: don't null test via == NULL unless LHS is a nested assignment expression in a loop condition), eliminating the if (prop) test and consolidating the prop-non-null code in a least-indented straight-line exit block.

> +        }
> +
> +        JSObject *config = arg.toObjectOrNull();
> +        JS_ASSERT(config);
> 
> This JS_ASSERT seems gratuitous, since we just called isObject a few lines
> above.

Indeed, so toObject() should be used (with address of the returned ref taken).

/be

	Dave Herman [:dherman] Assignee
	Comment 4 • 14 years ago

> +            JSString *str = js_ValueToString(cx, arg);
> +            const char *chars = js_GetStringBytes(NULL, str);
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
> JSMSG_UNEXPECTED_TYPE,
> +                                 chars, "not an object");
>              return JS_FALSE;
> 
> I think we generally use js_ReportValueErrorFlags here, so the expression
> appears in the error message.

This doesn't seem right. Printing out Reflect.parse(<some big string>, <some bogus second argument>) to give an error about the bogus second argument is dumping irrelevant info, particularly that potentially large first string.

> -var withFile = Reflect.parse("42", "foo.js");
> -var withFileAndLine = Reflect.parse("42", "foo.js", 111);
> +var withFile = Reflect.parse("42", {source:"foo.js"});
> +var withFileAndLine = Reflect.parse("42", {source:"foo.js", line:111});
> 
> How does this work?  You haven't set a 'loc' property, so shouldn't the other
> properties of the config object be ignored?

That's the point of the configuration object: all of its properties are optional, with sensible defaults.

Dave

	Dave Herman [:dherman] Assignee
	Comment 5 • 14 years ago

> That's the point of the configuration object: all of its properties are
> optional, with sensible defaults.

To clarify: loc defaults to true.

Dave

	Dave Herman [:dherman] Assignee
	Comment 6 • 14 years ago

> > I think we generally use js_ReportValueErrorFlags here, so the expression
> > appears in the error message.
> 
> This doesn't seem right.

But it would've, if I'd realized I needed to pass JSDVG_SEARCH_STACK for the fourth argument. Sorry about that.

Updated patch later today.

Dave

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 8 • 14 years ago

Comment on attachment 475532 [details] [diff] [review]
some code hygiene; uses js_ReportValueErrorFlags; copies filename string to local buffer

Stealing review.

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 9 • 14 years ago

Comment on attachment 475532 [details] [diff] [review]
some code hygiene; uses js_ReportValueErrorFlags; copies filename string to local buffer

+    if (!js_LookupPropertyWithFlags(cx, obj, id, JSRESOLVE_QUALIFIED, &obj2, &prop) < 0)
+        return false;

This is probably not what you want. Unary are higher precedence than relational ops in C, so this guard is a contradiction. Ditch the !-)

+static bool
+GetProperty(JSContext *cx, JSObject *obj, const char *name, Value *vp, bool *foundp)

I'm confused, why write this kind of thing at all?

For reading values off of a configuration object I'd just call |JS_GetProperty|, which already does this ugliness. (jsreflect should feel free to use API functions despite breaking the API boundary on parse stuff -- we can break out inline helpers where profiling indicates they'd be helpful in the future.)

How about getting rid of a custom class like |CharBuffer| and using a pre-canned function like |js_DeflateString|?

I'm betting I'm just pointing out the already-existing wheels.

	Dave Herman [:dherman] Assignee
	Comment 10 • 14 years ago

> +    if (!js_LookupPropertyWithFlags(cx, obj, id, JSRESOLVE_QUALIFIED, &obj2,
> &prop) < 0)
> +        return false;
> 
> This is probably not what you want. Unary are higher precedence than relational
> ops in C, so this guard is a contradiction. Ditch the !-)

D'oh. Thanks for catching that.

> +static bool
> +GetProperty(JSContext *cx, JSObject *obj, const char *name, Value *vp, bool
> *foundp)
> 
> I'm confused, why write this kind of thing at all?
> 
> For reading values off of a configuration object I'd just call
> |JS_GetProperty|, which already does this ugliness.

JS_GetProperty returns JSVAL_VOID when the property is not found. I need an operation that returns a result *and* tells you whether it was found. I could use JS_LookupProperty and JS_GetProperty together, but this abstracts that pattern.

The reason I need both is that |loc| defaults to *true* -- so it needs to tell the different between "falsey" and "not there".

Alternatively, I could negate the property name to something like |suppressLoc| and have it default to false. I'd only change this if it seemed like the better API, not to save a few lines in the implementation. Thoughts?

> How about getting rid of a custom class like |CharBuffer| and using a
> pre-canned function like |js_DeflateString|?

I could use js_DeflateString (which I agree would be better than my silly use of js_DeflateStringToBuffer) but IINM I'd still need to js_free it, right? I created CharBuffer to use RAII, since there are multiple early returns in the function.

Dave

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 11 • 14 years ago

We chatted about this a bit on IRC, Dave suggested we put it on the record.

(In reply to comment #10)
> The reason I need both is that |loc| defaults to *true* -- so it needs to tell
> the different between "falsey" and "not there".

Seems like adding a |JS_GetPropertyDefault(cx, obj, name, jsval *vp, jsval default)| for this configuration object pattern would be a useful API addition! stick the |default| value into |*vp| if the property isn't found and avoid redundant work through the Has/Get combo.

> IINM I'd still need to js_free it, right? I
> created CharBuffer to use RAII, since there are multiple early returns in the
> function.

jscntxt.h has an |AutoReleasePtr| type that you can use to perform a simple |cx->free| as RAII. (|cx->free| is the per-context-memory-accounting wrapper around |js_free|.)

	Dave Herman [:dherman] Assignee
	Comment 12 • 14 years ago

After more IRC conversation, just a private js::GetPropertyDefault for now. Can file a separate bug on exposing that as a public API.

Dave

	Brendan Eich [:brendan]
	Comment 13 • 14 years ago

Why back off from extending the JS API? I liked the idea, and it sounds like it fills a real gap.

We aren't going to get a better C++ API by hiding a C-like entry point in namespace js, so I say share the wealth. Cc'ing jorendorff for his thoughts.

/be

	Dave Herman [:dherman] Assignee
	Comment 14 • 14 years ago

Addresses the points from the above discussion.

Dave

	Dave Herman [:dherman] Assignee
	Comment 15 • 14 years ago

Oops, added a use of isNullOrUndefined instead of two separate isNull/isUndefined checks.

Dave

	Dave Herman [:dherman] Assignee
	Comment 16 • 14 years ago

Added a JSAPI test for JS_GetPropertyDefault and JS_GetPropertyByIdDefault.

Dave

	Chris Leary [:cdleary] (not checking bugmail)
	Comment 17 • 14 years ago

Comment on attachment 481941 [details] [diff] [review]
adds jsapi test for JS_GetProperty{ById}Default

+        jsval v2;
+        CHECK(JS_GetPropertyDefault(cx, obj, "nothere", JSVAL_FALSE, &v2));
+        CHECK(!JSVAL_TO_BOOLEAN(v2));

JSVAL_TO_BOOLEAN isn't technically enough to distinguish JSVAL_FALSE from JSVAL_VOID in opt builds -- it will always assert bool-ness in debug, but you have undefined behavior in opt -- probably want to throw a JSVAL_IS_BOOLEAN check in there as well for safety.

+    uintN lineno = 1;

I think we prefer int32/uint32 nowadays to the *N integer type variants.

+        JSObject *config = &arg.toObject();
+        JS_ASSERT(config);

|toObject| is already returning you a reference to indicate non-nullness, so the assertion is redundant. (Note that on the flip side of this API is |toObjectOrNull| which does not assert out on a NULL value and returns a pointer, as opposed to a ref).

+                filename = js_DeflateString(cx, str->chars(), str->length());
+                if (!filename)
+                    return JS_FALSE;

This isn't going to be enough to trigger a release from the auto_ptr -- since you initialized it with the pointer value of null (note: the *value* of filename, as opposed to a reference to it), changing the value of the pointer after the fact won't cause a release. You need to add a |.reset(void *ptr)| method to the auto_ptr to reinitialize the internal pointer value. In the STL, the |auto_ptr<T>.reset(T *)| also deallocates the currently held pointer -- I would go with that API for familiarity's sake.

r=me with those points addressed.

	Dave Herman [:dherman] Assignee
	Comment 18 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/79a5778150b6

	Brendan Eich [:brendan]
	Comment 19 • 14 years ago

(In reply to comment #17)
> Comment on attachment 481941 [details] [diff] [review]
> adds jsapi test for JS_GetProperty{ById}Default
> 
> +        jsval v2;
> +        CHECK(JS_GetPropertyDefault(cx, obj, "nothere", JSVAL_FALSE, &v2));
> +        CHECK(!JSVAL_TO_BOOLEAN(v2));
> 
> JSVAL_TO_BOOLEAN isn't technically enough to distinguish JSVAL_FALSE from
> JSVAL_VOID in opt builds -- it will always assert bool-ness in debug, but you
> have undefined behavior in opt -- probably want to throw a JSVAL_IS_BOOLEAN
> check in there as well for safety.
> 
> +    uintN lineno = 1;
> 
> I think we prefer int32/uint32 nowadays to the *N integer type variants.

uintN is still used for lineno and other "32 bits or better", "native int size" variables. It's not important to pin down 32 vs. 64 bits, and pinning to 32 other than when packing in structs has the potential to hurt performance on some 64-bit targets.

/be

	Robert Sayre
	Comment 20 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/79a5778150b6