Thanks for the feedback. Please take another look.
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);
On 2011/09/08 12:06:02, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1224
src/runtime.cc:1224: continue;
On 2011/09/08 12:06:02, Kevin Millikin wrote:
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.
Done.
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.
On 2011/09/08 12:06:02, Kevin Millikin wrote:
Let's change all the mentions of declaration, redeclaration, etc. in
the
comments to say 'initialization', 're-initialization', etc.
Done.
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.
On 2011/09/08 12:06:02, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1488
src/runtime.cc:1488: // Determine if this is a redeclaration of
something not
On 2011/09/08 12:06:02, Kevin Millikin wrote:
Not your code, but the comments that mention 'declaration' when this
function is
doing initialization are confusing and need to be changed.
Done.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1493
src/runtime.cc:1493: return isolate->heap()->undefined_value();
On 2011/09/08 12:06:02, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/7811015/diff/1/src/runtime.cc#newcode1512
src/runtime.cc:1512: // uninitialized, e.g. the hole. Nirk...
On 2011/09/08 12:06:02, Kevin Millikin wrote:
I'm not sure how to interpret "nirk", but it doesn't inspire much
confidence.
I could change it to "nurk", which I think sounds more confident.
http://codereview.chromium.org/7811015/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
