LGTM if the comments below are addressed.
http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): http://codereview.chromium.org/7736018/diff/1/src/arm/full-codegen-arm.cc#newcode777 src/arm/full-codegen-arm.cc:777: __ mov(r1, Operand(Smi::FromInt(is_eval() ? 1 : 0))); Don't you have to remove this argument? http://codereview.chromium.org/7736018/diff/1/src/messages.js File src/messages.js (right): http://codereview.chromium.org/7736018/diff/1/src/messages.js#newcode32 src/messages.js:32: const TYPE_NATIVE = 0; Is this change necessary? If it is, can you instead define them as const macros in macros.py? Our implementation of const is slower than var, prevents optimization, and the semantics of global const is somewhat up in the air and not something we should rely on. http://codereview.chromium.org/7736018/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1160 src/runtime.cc:1160: bool is_eval = (flags & kDeclareGlobalsEvalFlag) != 0; These flags and 'base' below are declared a away from where they're used and this is a big function. You might consider moving them down toward their use. If you keep the declarations up here, can you make them const so the reader knows he doesn't have to look at all the intervening code between definition and use? I also think it's clearer to make strict_mode a bool and only convert it to the enum at its single use. http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1246 src/runtime.cc:1246: PropertyAttributes attributes = Here, you could just get rid of base and have the more direct: int attr = NONE; if (!is_eval) attr |= DONT_DELETE; if (is_const_property || (is_native && is_function_declaration)) attr |= READ_ONLY; You can also move it down below the next check that doesn't use the attributes. http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1275 src/runtime.cc:1275: attributes = static_cast<PropertyAttributes>( Here attr |= (lookup.GetAttributes() & DONT_DELETE); http://codereview.chromium.org/7736018/diff/1/src/runtime.cc#newcode1278 src/runtime.cc:1278: RETURN_IF_EMPTY_HANDLE(isolate, Here: SetLocalPropertyIgnoreAttributes(global, ... static_cast<PropertyAttributes>(attr)); http://codereview.chromium.org/7736018/diff/1/src/v8globals.h File src/v8globals.h (right): http://codereview.chromium.org/7736018/diff/1/src/v8globals.h#newcode485 src/v8globals.h:485: kNonStrictMode = 0, I couldn't find a site where we relied on a specific values for kNonStrictMode or kStrictMode, so I'd rather not make this change. It suggests the reader here that we already *do* rely on a specific value and so encourages him to make use of that fact. There doesn't seem a good reason to introduce that dependency. http://codereview.chromium.org/7736018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
