Closed Bug 551595 Opened 15 years ago Closed 15 years ago

TM: public JS API for new ES5 Object functions

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

No description provided.
Depends on: harmony:proxies
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
JS_ES5 prefix? Yeesh. js_GetOwnPropertyDescriptorById would be the better name, then have the JS_GetOwnPropertyDescriptor public version in jsapi.cpp just call that.
Look into jsapi.h before complaining. Some of the names are already taken, and the file is extern "C", so we can't overload.
(In reply to comment #3) > Look into jsapi.h before complaining. Some of the names are already taken, and > the file is extern "C", so we can't overload. Waldo's right, this is wrong. There's no existing conflict. This grep is from my mq pushed on top of tip tm: $ grep Descriptor *.h|grep JS_ jsapi.h:JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN flags, jscntxtinlines.h: JS_CALL_VALUE_TRACER(trc, desc.value, "PropertyDescriptor::value"); jscntxtinlines.h: JS_CALL_VALUE_TRACER(trc, desc.get, "PropertyDescriptor::get"); jscntxtinlines.h: JS_CALL_VALUE_TRACER(trc, desc.set, "PropertyDescriptor::set"); Please do not add ES5_ middle-fix to API names. /be
You are both wrong. Brendan even quoted it. extern JS_PUBLIC_API(JSBool) JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN flags, JSPropertyDescriptor *desc); The equivalent JS function is Object.prototype.getPropertyDescriptor. We want it to return a jsval. The existing API function returns a JSPropertyDescriptor *. I can't overload it. We can: - not mention the ById, but that's inconsistent with existing naming and confusing. - inject "Object", but that makes the naming out of sync with Object.* (GetPropertyDescriptorObjectById) - make jsapi.h C++ I am fine with either. The ES5 was a placeholder until we have a better fix. But there is a a conflict.
I wonder if it's worth just changing JS_GetPropertyDescriptorById. It's a new API as of not too long ago.
(In reply to comment #5) > You are both wrong. Brendan even quoted it. > > extern JS_PUBLIC_API(JSBool) > JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, uintN > flags, > JSPropertyDescriptor *desc); > > The equivalent JS function is Object.prototype.getPropertyDescriptor. So? Either rename the old function as mrbkap suggested, or drop the "ById" noise and give your better new API the plain name. There's no conflict if you don't insist on JS_ES5_InsanelyLongNamesById :-|. /be
Attached patch patch (obsolete) — Splinter Review
Attachment #431756 - Attachment is obsolete: true
Attachment #432637 - Flags: review?(jwalden+bmo)
No longer depends on: harmony:proxies
review ping
Attached patch patch (obsolete) — Splinter Review
refreshed patch
Attachment #432637 - Attachment is obsolete: true
Attachment #433116 - Flags: review?(jwalden+bmo)
Attachment #432637 - Flags: review?(jwalden+bmo)
Attachment #433116 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 433116 [details] [diff] [review] patch >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >+JS_PUBLIC_API(JSBool) >+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsval name, jsval *vp) >+{ As a diff-reducing measure it's acceptable to define this in jsobj.cpp, but it really should go in jsapi.cpp; likewise for JS_DefineOwnProperty. (It's helpful to know that a JS_* function is always in jsapi.cpp; the current decimal-to-string exceptions that aren't -- at least for the moment -- are very confusing.) Followup bug, or (even better) followup checkin immediately after this one? > AutoIdRooter nameidr(cx); >- if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) >+ if (!JS_ValueToId(cx, name, nameidr.addr())) > return JS_FALSE; |name| should be jsid, not jsval, to save this effort in cases where a name has already been id-ified. > static JSBool >+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp) >+{ >+ if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) { >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT); >+ return false; >+ } >+ >+ JSObject *obj = JSVAL_TO_OBJECT(vp[2]); >+ >+ jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID; >+ >+ return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp); >+} The empty lines between the last three statements are a bit odd; remove them. >+JS_PUBLIC_API(JSBool) >+JS_DefineOwnProperty(JSContext *cx, JSObject *obj, jsval name, jsval descriptor) >+{ >+ AutoIdRooter tvr(cx); >+ AutoDescriptorArray descs(cx); >+ PropertyDescriptor *desc = descs.append(); >+ >+ if (!JS_ValueToId(cx, name, tvr.addr()) || >+ !desc || >+ !desc->initialize(cx, tvr.id(), descriptor)) { >+ return false; >+ } You should test desc before attempting to convert to id; this could double-report -- but changing to jsid would fix that anyway, because the ->id conversion would be unnecessary. But a more basic question: is it acceptable for JS_DefineOwnProperty to return a success value but not actually define the property? Your dummy variable right now means that a success value doesn't actually mean the property was defined. Is that acceptable? I'm thinking it isn't, and that we should add a JSBool outparam for whether a property was defined or not. r- particularly to address this issue.
> >+JS_PUBLIC_API(JSBool) > >+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsval name, jsval *vp) > >+{ > > As a diff-reducing measure it's acceptable to define this in jsobj.cpp, but it > really should go in jsapi.cpp; likewise for JS_DefineOwnProperty. (It's > helpful to know that a JS_* function is always in jsapi.cpp; the current > decimal-to-string exceptions that aren't -- at least for the moment -- are very > confusing.) Followup bug, or (even better) followup checkin immediately after > this one? IM(H) API entry points should be located where they can inline and access the definitions they need. The new API mirror jsobj operations and should be located there. No need to take inlining/linking overhead just so everything JS_ is in one file. That's what the lord in his infinite wisdom gaves us grep for :) > > > > AutoIdRooter nameidr(cx); > >- if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) > >+ if (!JS_ValueToId(cx, name, nameidr.addr())) > > return JS_FALSE; > > |name| should be jsid, not jsval, to save this effort in cases where a name has > already been id-ified. > Yeah, that's cleaner. I wasn't sure we want to export jsid hence my somewhat awkward signature. Fixing that. > > > > static JSBool > >+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp) > >+{ > >+ if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) { > >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT); > >+ return false; > >+ } > >+ > >+ JSObject *obj = JSVAL_TO_OBJECT(vp[2]); > >+ > >+ jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID; > >+ > >+ return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp); > >+} > > The empty lines between the last three statements are a bit odd; remove them. > > > >+JS_PUBLIC_API(JSBool) > >+JS_DefineOwnProperty(JSContext *cx, JSObject *obj, jsval name, jsval descriptor) > >+{ > >+ AutoIdRooter tvr(cx); > >+ AutoDescriptorArray descs(cx); > >+ PropertyDescriptor *desc = descs.append(); > >+ > >+ if (!JS_ValueToId(cx, name, tvr.addr()) || > >+ !desc || > >+ !desc->initialize(cx, tvr.id(), descriptor)) { > >+ return false; > >+ } > > You should test desc before attempting to convert to id; this could > double-report -- but changing to jsid would fix that anyway, because the ->id > conversion would be unnecessary. > > But a more basic question: is it acceptable for JS_DefineOwnProperty to return > a success value but not actually define the property? Your dummy variable > right now means that a success value doesn't actually mean the property was > defined. Is that acceptable? I'm thinking it isn't, and that we should add a > JSBool outparam for whether a property was defined or not. r- particularly to > address this issue. Fixing these. New patch in a sec.
Note that I have an id now, but the name is not ById. I like this (we don't want to add const char* args in the future, so we could start dropping the ById). But its inconsistent. Make sure you are happy with the next patch. We will have to live with the api for a while. I am not married to the naming either way.
just FYI: host-3-164:src gal$ grep -l "JS_PUBLIC" *.cpp jsapi.cpp jsarena.cpp jscntxt.cpp jsdbgapi.cpp jsdhash.cpp jsgc.cpp jshash.cpp jslog2.cpp jsobj.cpp jsprf.cpp jsutil.cpp jsxdrapi.cpp
Attached patch patch (obsolete) — Splinter Review
Attachment #433116 - Attachment is obsolete: true
Attachment #433674 - Flags: review?(jwalden+bmo)
(In reply to comment #14) > just FYI: > > host-3-164:src gal$ grep -l "JS_PUBLIC" *.cpp > jsapi.cpp > jsarena.cpp > jscntxt.cpp > jsdbgapi.cpp > jsdhash.cpp > jsgc.cpp > jshash.cpp > jslog2.cpp > jsobj.cpp > jsprf.cpp > jsutil.cpp > jsxdrapi.cpp History: NSPR started as a separate library used by the JS engine, then around 1998 it was forked into js/src. So js{arena,hash,log2,prf,util}.cpp were from the original NSPR (more or less). Same would apply to jsdhash.cpp if I'd added it to NSPR 1. This leaves js{,dbg,xdr}api.cpp whose names all shout API, and jscntxt.cpp and jsgc.cpp, which have historically very recent JS_PUBLIC_API entry points defined in them. So what, you ask? Good question. The objection to defining new API all over could probably be overcome with static inline locals that you want to inline-expand in the API entry point, but is that an issue here? If not, or if it can be avoided, use jsapi.cpp. There are competing values of inline-ability vs. one stop API shopping at work. /be
2:1 I lose. I will move it into jsapi. Waldo you can r+ based on that nit if you want.
The "one stop API shopping" or "APIs on the outside" is a value because it tends (no guarantees, but tendencies and norms help) to keep API disentangled from implementation. JSC and V8 have APIs segregated (IIRC, V8 patterned after JSC) and JSC at least, Ollie Hunt tells me, has strict ABI compatibility constraints on its exported API. I've been talking up a big source reorg, perhaps when JaegerMonkey lands, so that could be a good time to further separate API from implementation. Inline-ability can be helped by jsfooinlines.h, or FooInlines.h in the StudlyCaps file naming convention. ABI compatibility may not matter to us right now, but it could come to matter with ctypes, popular/decoupled platform consumers (add-ons), or our own desires to field-upgrade just the engine. It's not a goal now but it could be, and it is easier to do with segregated API sources. /be
Comment on attachment 433674 [details] [diff] [review] patch >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp > static JSBool >+obj_getOwnPropertyDescriptor(JSContext *cx, uintN argc, jsval *vp) >+{ >+ if (argc == 0 || JSVAL_IS_PRIMITIVE(vp[2])) { >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT); >+ return false; >+ } >+ JSObject *obj = JSVAL_TO_OBJECT(vp[2]); >+ jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID; >+ return JS_GetOwnPropertyDescriptor(cx, obj, nameval, vp); >+} You misunderstand the distinction between jsval and jsid. There can be umpteen different jsvals which all correspond to a JSString consisting of the characters "foobar", because there can be multiple such strings. There can only be one jsid which corresponds to the characters "foobar"; if you had more, that's going to start messing things up in unpredictable ways. (Here, you might get multiple properties of the same name!) jsval and jsid might be type-compatible in C++, but they aren't the same, and you can't safely pass in a jsval where a jsid is expected. You need this bit that you cut out from the original code to get a proper jsid: - AutoIdRooter nameidr(cx); - if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) - return JS_FALSE; > /* ES5 15.2.3.6: Object.defineProperty(O, P, Attributes) */ > static JSBool > obj_defineProperty(JSContext* cx, uintN argc, jsval* vp) > { > /* 15.2.3.6 steps 1 and 5. */ > jsval v = (argc == 0) ? JSVAL_VOID : vp[2]; > if (JSVAL_IS_PRIMITIVE(v)) { > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT); > return JS_FALSE; > } > *vp = vp[2]; > JSObject* obj = JSVAL_TO_OBJECT(*vp); > > /* 15.2.3.6 step 2. */ >- AutoIdRooter nameidr(cx); >- if (!JS_ValueToId(cx, argc >= 2 ? vp[3] : JSVAL_VOID, nameidr.addr())) >- return JS_FALSE; >+ jsval nameval = argc >= 2 ? vp[3] : JSVAL_VOID; > > /* 15.2.3.6 step 3. */ >- AutoDescriptorArray descs(cx); >- PropertyDescriptor *desc = descs.append(); >- if (!desc || !desc->initialize(cx, nameidr.id(), argc >= 3 ? vp[4] : JSVAL_VOID)) >- return JS_FALSE; >+ jsval descval = argc >= 3 ? vp[4] : JSVAL_VOID; > > /* 15.2.3.6 step 4 */ >- bool dummy; >- return DefineProperty(cx, obj, *desc, true, &dummy); >+ JSBool junk; >+ return JS_DefineOwnProperty(cx, obj, nameval, descval, &junk); Same thing again here.
Attachment #433674 - Flags: review?(jwalden+bmo) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #433674 - Attachment is obsolete: true
Attached patch patchSplinter Review
I just caught a bad bug the review didn't flag, so please look again carefully. Stuff shifted around a lot the last few review cycles.
Attachment #434137 - Attachment is obsolete: true
Attachment #434140 - Flags: review?(jwalden+bmo)
Comment on attachment 434140 [details] [diff] [review] patch >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp > JS_PUBLIC_API(JSBool) >+JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, jsval *vp) >+{ >+ CHECK_REQUEST(cx); >+ >+ return js_GetOwnPropertyDescriptor(cx, obj, id, vp); >+} Nix the newline here. >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >+JSBool >+js_DefineOwnProperty(JSContext *cx, JSObject *obj, jsid id, jsval descriptor, JSBool *bp) >+{ >+ AutoDescriptorArray descs(cx); >+ PropertyDescriptor *desc = descs.append(); >+ >+ if (!desc || >+ !desc->initialize(cx, id, descriptor)) { >+ return false; >+ } Nix the newline, make that a single-line if without braces. >+ bool rval; >+ if (!DefineProperty(cx, obj, *desc, true, &rval)) >+ return false; >+ *bp = !!rval; >+ return true; Were you not looking at compiler warning output here?
Attachment #434140 - Flags: review?(jwalden+bmo) → review+
> Were you not looking at compiler warning output here? What do you mean?
I meant with respect to previous versions of the patch, where |*bp = !!rval| immediately followed a return statement.
It was a mis-merge from rebasing. Did you not look at the patch you reviewed? =)
Whiteboard: fixed-in-tracemonkey
I looked, and I r-'d it. :-) Regarding the specific spot, I assume patch writers take care to eliminate compiler warnings before posting patches, because that's usually the case and the trouble testing it usually doesn't pay off. But the point still stands: check patches for compiler warnings.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 566818
No longer blocks: 566818
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: