New version uploaded.
http://codereview.chromium.org/463040/diff/5/1006 File src/messages.js (right): http://codereview.chromium.org/463040/diff/5/1006#newcode162 src/messages.js:162: value_and_getter_setter: "Invalid property. 'value' present on property with getter or setter", On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > This sounds confusing. How about something along the lines of "A property > cannot have both a value and accessors". I'm matching the error messages from JSC here. I agree they are not ideal. http://codereview.chromium.org/463040/diff/5/1006#newcode163 src/messages.js:163: proto_object_or_null: 'Object prototype may only be an Object or null', On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > Inconsistent string delimiters. Fixed. http://codereview.chromium.org/463040/diff/5/1005 File src/v8natives.js (right): http://codereview.chromium.org/463040/diff/5/1005#newcode286 src/v8natives.js:286: if (desc === void 0) return false; On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > If you're deliberately avoiding IS_UNDEFINED please document why. Otherwise you > should use IS_UNDEFINED. Fixed. http://codereview.chromium.org/463040/diff/5/1005#newcode316 src/v8natives.js:316: var enumerable = obj.enumerable; On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > This doesn't seem to match what's in the spec. Shouldn't it be something like > var enum = false; > if ('enumerable' in obj) { > enum = ToBoolean(obj.enumerable); > } > Ditto, with variations, for every block below. In any case |enumerable in obj| > certainly seems incorrect, it should be |"enumerable" in obj|. Certainly it was wrong, thanks for pointing out the missing ToBoolean and quotes in the 'in' expression. The reason why I explicitly check for undefined is that we don't have an inline cache for 'in' so in the case where the property is present and not undefined we skip the 'in'. http://codereview.chromium.org/463040/diff/5/1005#newcode360 src/v8natives.js:360: this.value_ = void 0; On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > The spec says nothing about these properties being present on property > descriptors. This is both a problem in and of itself but it also allows people > to put a property descriptor into an inconsistent state. These property descriptors are never exposed to the user. The spec does not give any details on how they should be implemented. The FromPropertyDescriptor method in 8.10.4 makes a new object with a well-defined structure which is passed to the user. Also, I can't find the > style guide right now but I believe variables should be lowerCamelCase. This > applies throughout. In general, use functionNamesLikeThis, variableNamesLikeThis, ClassNamesLikeThis, EnumNamesLikeThis, methodNamesLikeThis, and SYMBOLIC_CONSTANTS_LIKE_THIS. (For legacy code, method names should start with uppercase). Getters and setters for properties are not required. However, if they are used, then getters must be named getFoo() and setters must be named setFoo(value). (For Boolean getters, isFoo() is also acceptable, and often sounds more natural.) Private properties, variables, and methods (in files or classes) should be named with a trailing underscore I'll fix this. http://codereview.chromium.org/463040/diff/5/1005#newcode440 src/v8natives.js:440: for (var key in props) { On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > This will include properties in the prototype chain of 'properties', only own > properties should be included (15.2.3.7 step 3). Good catch. http://codereview.chromium.org/463040/diff/5/1004 File test/mjsunit/object-create.js (right): http://codereview.chromium.org/463040/diff/5/1004#newcode43 test/mjsunit/object-create.js:43: var Foo = { foo: function() { ctr++; }}; On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > Peculiar naming convention -- Foo is not a function. Actually foo is very like a superclass and is used as one so I think it's OK that it's named like a class. The others got their naming convention by just riding on Foo's coat-tails and they are now fixed. http://codereview.chromium.org/463040/diff/5/1004#newcode184 test/mjsunit/object-create.js:184: print("ok"); On 2009/12/08 04:30:52, Christian Plesner Hansen wrote: > Excuse me? The JSC shell silently terminates on exceptions (at least the way I build it...) so this is mighty convienient. It doesn't seem to hurt. I suppose I could surround the whole thing with a try-catch, but then the non-optimizing compiler wouldn't get tested. http://codereview.chromium.org/463040 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
