Property getters and setters is a Firefox extension. It's described at http://developer.mozilla.org/en/docs/ Core_JavaScript_1.5_Guide:Working_with_Objects#Defining_Getters_and_Setters
Created attachment 5027 [details] Implement getters and setters Note that this does not implement the old deprecated way of defining getters and setters (which unfortunately is used in the jsc test suite)
Comment on attachment 5027 [details] Implement getters and setters Ah, some tests actually use __defineGetter__ and __defineSetter__ but still fail, I'll investigate why
+ if (params) + func = new FuncExprNode(Identifier::null(), params, body); + else + func = new FuncExprNode(Identifier::null(), body); This code seemed a little bizarre to me at first read. Since params is an optional argument, why isn't it the last argument? I've attached a manual patch to show you what I mean. (CVS is down for me right now.) It has the added advantage of collapsing the above lines to one: func = new FuncExprNode(Identifier::null(), body, params);
Created attachment 5034 [details] Make params an optional, last argument
+JSValue *GetterSetterImp::toPrimitive(ExecState *, Type) const +{ + assert(false); + return 0; +} +JSObject *GetterSetterImp::toObject(ExecState *exec) const +{ + assert(false); + return 0; +} JavaScriptCore is not tolerant of null pointers, so these return values for deployment builds will almost certainly cause a crash if they're ever hit. I would suggest return jsNull().toPrimitive(exec); and return jsNull().toObject(exec);
Same comment here: +JSValue *PropertyNode::evaluate(ExecState *exec) +{ + assert(false); + return 0; +}
Created attachment 5055 [details] Address comments Here's a new patch that addresses the comments made, and also passes getset-00[456].js in the mozilla test suite. I had to do some changes to PropertySlot which makes the patch big, but Maciej thought it was the right approach (and so do I)
Comment on attachment 5055 [details] Address comments In PropertyListNode::evaluate, defineGetter and defineSetter are called on the value from the assign node without checking that it's a JSObject. That's dangerous and could crash if the thing is not an object. The code needs a check if it's an object somewhere rather than just calling JSObject member functions on a JSValue. I notice this line in the parser is no longer lined up with the code above it: + | '{' PropertyList '}' { $$ = new ObjectLiteralNode($2); } I know that Geoff suggested returning jsNull()->toPrimitive(exec, type), but that's exactly the same as jsNull(), so there's no reason to include the extra function call. I also think that the logical equivalent of assert(false) is to call throwError so we get a JavaScript exception in those cases. But since these values should never get out of properties into expressions, there's very little reason to worry about exactly what code is there after the assertion, never to be executed. Is PropertyNode::streamTo really all that hard to write for getter and setter? I like to see member functions that are virtual re-declared as virtual, so I'd like to see that in GetterSetterImp's declaration. The PropertyListNode::evaluate seems to be a serious one, so marking review-, otherwise I'd say review+.
Created attachment 5066 [details] Address more comments (In reply to comment #8) > (From update of attachment 5055 [details] [edit]) > In PropertyListNode::evaluate, defineGetter and defineSetter are called on the > value from the assign node without checking that it's a JSObject. That's > dangerous and could crash if the thing is not an object. The code needs a check > if it's an object somewhere rather than just calling JSObject member functions > on a JSValue. > The reason I skipped this check is that we know that the node to be evaluated always is a FuncExprNode which will always be evaluated to a JSObject. Anyway, I've added an isObject check for both the getter and setter. > I notice this line in the parser is no longer lined up with the code above it: > > + | '{' PropertyList '}' { $$ = new ObjectLiteralNode($2); } > > I know that Geoff suggested returning jsNull()->toPrimitive(exec, type), but > that's exactly the same as jsNull(), so there's no reason to include the extra > function call. I also think that the logical equivalent of assert(false) is to > call throwError so we get a JavaScript exception in those cases. But since > these values should never get out of properties into expressions, there's very > little reason to worry about exactly what code is there after the assertion, > never to be executed. > Both of these have been fixed. > Is PropertyNode::streamTo really all that hard to write for getter and setter? It's not hard to write, but the end result will look ugly because there will be newlines everywhere. I added an implementation for this that will generate ugly, yet syntactically correct, text. > > I like to see member functions that are virtual re-declared as virtual, so I'd > like to see that in GetterSetterImp's declaration. > Fixed. > The PropertyListNode::evaluate seems to be a serious one, so marking review-, > otherwise I'd say review+. >
Comment on attachment 5066 [details] Address more comments I noticed that the JSC ChangeLog is messed up; I've fixed that in my local copy.
Comment on attachment 5066 [details] Address more comments As far as PropertyListNode::evaluate is concerned -- if we really do know the value is an object, then it can be an assert rather than an if. r=me