http://codereview.chromium.org/2123012/diff/1/6 File src/objects.cc (right):
http://codereview.chromium.org/2123012/diff/1/6#newcode2774 src/objects.cc:2774: || Top::MayNamedAccess(this, name, v8::ACCESS_SET)); Do the types match here? I think you check that HAS access is allowed and here you assert that SET access is allowed. Should you be checking SET access below? http://codereview.chromium.org/2123012/diff/1/6#newcode2879 src/objects.cc:2879: !Top::MayNamedAccess(this, name, v8::ACCESS_HAS)) { Should you check for SET instead of HAS here? http://codereview.chromium.org/2123012/diff/1/6#newcode2935 src/objects.cc:2935: if (result.IsProperty() && (result.IsReadOnly() || result.IsDontDelete())) { Why do we check for DontDelete when setting an accessor property? I guess it is up to your interpretation of the action of setting an accessor but to me that does not seem like a deletion. If this is the behavior that we want, we should make sure to document it. Also, I don't understand the comment below. What are the two DefineAccessors you are talking about? __defineGetter__ and __defineSetter__ defined accessors? Example? http://codereview.chromium.org/2123012/diff/1/6#newcode2936 src/objects.cc:2936: // That's somewhat unfortunate that two DefineAccessors That's -> It is http://codereview.chromium.org/2123012/diff/1/6#newcode2937 src/objects.cc:2937: // diff here, but another one performs DON_DELETE check another one -> the other one http://codereview.chromium.org/2123012/diff/1/6#newcode5966 src/objects.cc:5966: Handle<Object> number = Factory::NewNumberFromUint(index); This code does not seem GC safe. |self| and |holder_handle| are raw pointers, but the number allocation could cause a heap allocation and thereby a garbage collection. http://codereview.chromium.org/2123012/diff/1/6#newcode5994 src/objects.cc:5994: return 0; NULL or maybe just undefined? http://codereview.chromium.org/2123012/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
