It's not your code, but the existing code is kind of convoluted. I've
highlighted some parts where I don't think we're doing what we intend with
this
change.
I'll try to figure out what we intend with the interceptors (it's a little
troubling at first glance that we don't have the same stuff for initializing
const context slots that end up hitting the global object).
Ultimately, but not as part of this change, we should change the whole
thing to
more closely follow the algorithms in the 5.1 spec.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1187
src/runtime.cc:1187: PropertyAttributes attributes =
global->GetPropertyAttribute(*name);
I think you could probably simplify this code a bit (and avoid calling
GetPropertyAttributes unless you need to, because it will repeat the
lookup).
// We found an existing property. Unless it was an interceptor
// that claims the property is absent, skip this declaration.
if (lookup.type() != INTERCEPTOR) continue;
PropertyAttributes attributes = ...;
if (attributes != ABSENT) continue;
Or some such.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1224
src/runtime.cc:1224: continue;
I don't think this is what we intend. Won't we skip a function
declaration following a const declaration?
const f = 0;
function f() { ... }
Because when processing the function declaration, there is a local
property with that name and it's read only?
I think we can get rid of this whole test+continue (and the completely
confusing comments before it). In the declaring a var/const case, we've
already skipped these (the global lookup would have found this local
property, and we skip all where != INTERCEPTOR, not just some of them
like here). In the declaring a function case, we don't want to skip
them.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1399
src/runtime.cc:1399: // Determine if this is a redeclaration of
something read-only.
Let's change all the mentions of declaration, redeclaration, etc. in the
comments to say 'initialization', 're-initialization', etc.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1408
src/runtime.cc:1408: // property and figure out if the property exists
at all.
This code is pretty complicated. I think what it's trying to do (after
the redeclaration stuff is removed) is see if there is a non-ABSENT,
non-READ_ONLY interceptor on the global object or one of its hidden
prototypes. If so, set the property on that hidden prototype.
If not, set the property on the global object itself. I think it should
be fine to go ahead and try to do that even for READ_ONLY properties on
the global object itself, they should just be ignored by SetProperty.
Could we rewrite it as something more direct? We have to be very
careful because there is no handle scope except in the case we see an
interceptor, and so we have to "export" handles from that handle scope
in case there was a GC (e.g., real_holder in the existing code) or look
them up repeatedly (e.g. isolate->context()->global()).
Object* object = global;
while (object->IsJSObject() &&
JSObject::cast(object)->map()->is_hidden_prototype()) {
real_holder->LocalLookup(*name, &lookup);
if (lookup.IsProperty() && lookup.type() == INTERCEPTOR) {
HandleScope ...
// etc.;
if (intercepted != ABSENT && (intercepted & READ_ONLY) == 0) {
// Found an interceptor that's not read only.
if (assign) {
return real_holder->SetProperty(...);
} else {
return <undefined>;
}
object = JSObject::cast(object)->GetPrototoype();
}
// Here you can set the property on global itself.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1488
src/runtime.cc:1488: // Determine if this is a redeclaration of
something not
Not your code, but the comments that mention 'declaration' when this
function is doing initialization are confusing and need to be changed.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1493
src/runtime.cc:1493: return isolate->heap()->undefined_value();
I'm not sure this is our intended semantics. Do we really want to
ignore the const initialization below?
eval("Object.setOwnPropertyDescriptor(this, 'x', { writable: true });"
+ "const x = 7; x");
The const declaration will introduce a property x with value undefined,
the const initialization will be ignored because the property is not
read only. Hmmm.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1512
src/runtime.cc:1512: // uninitialized, e.g. the hole. Nirk...
I'm not sure how to interpret "nirk", but it doesn't inspire much
confidence.
http://codereview.chromium.org/7811015/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
