LGTM
http://codereview.chromium.org/6594037/diff/1/src/api.cc File src/api.cc (right): http://codereview.chromium.org/6594037/diff/1/src/api.cc#newcode2290 src/api.cc:2290: i::kNonStrictMode); // TODO(mmaly): v8::Object::Set API. Needs strict? I'd say to keep it non-strict. The C++-API isn't part of the language, but is a separate way to work on the JS data. We can reuse the non-strict JS functionality for that because it doesn't add restrictions. http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc File src/ia32/ic-ia32.cc (right): http://codereview.chromium.org/6594037/diff/1/src/ia32/ic-ia32.cc#newcode1637 src/ia32/ic-ia32.cc:1637: __ TailCallRuntime(Runtime::kSetProperty, 5, 1); You could combine the values into a "StoreICFlag" encoded value (e.g., using BitFields), but this should be fine. http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#newcode2555 src/ia32/stub-cache-ia32.cc:2555: __ push(Immediate(Smi::FromInt(strict_mode_))); // strict mode Comment isn't necessary here. The variable is self-explanatory :). http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#newcode3720 src/ia32/stub-cache-ia32.cc:3720: : kNonStrictMode))); Is the conditional necessary here? It seems Code::ExtractExtraICStateFromFlags(flags) returns kStrictMode directly (or at most something castable to it). http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc File src/ia32/virtual-frame-ia32.cc (right): http://codereview.chromium.org/6594037/diff/1/src/ia32/virtual-frame-ia32.cc#newcode1042 src/ia32/virtual-frame-ia32.cc:1042: strict_mode == kStrictMode ? Builtins::StoreIC_Initialize_Strict Put parentheses around any non-trivial condition of a ?: operator. http://codereview.chromium.org/6594037/diff/1/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1868 src/ic.cc:1868: // I don't know. Do you have a way to reproduce the problem? http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1946 src/ic.cc:1946: // Again, can you reproduce it? That would make it much easier to figure out who to ask about it :) http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1947 src/ic.cc:1947: // ASSERT(extra_ic_state == ic.target()->extra_ic_state()); Should be removed before commit. http://codereview.chromium.org/6594037/diff/1/src/ic.h File src/ic.h (right): http://codereview.chromium.org/6594037/diff/1/src/ic.h#newcode436 src/ic.h:436: ASSERT(code->extra_ic_state() == target()->extra_ic_state()); If the "extra_ic_state" is only strict/non-strict, just call it strict_ic_state and return the correct enum value directly. The way you use it seems to assume that the value is exactly kStrictMode. If we ever need more state, we can add more accessors. http://codereview.chromium.org/6594037/diff/1/src/parser.cc File src/parser.cc (right): http://codereview.chromium.org/6594037/diff/1/src/parser.cc#newcode1658 src/parser.cc:1658: temp_scope_->StrictMode() ? kStrictMode : kNonStrictMode)); I don't think so. The NumberLiteral will hold a smi, so it won't make Heap allocation, only Zone allocation. http://codereview.chromium.org/6594037/diff/1/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode550 src/runtime.cc:550: // Passing non-strict per Ecma-262 5th Ed. 12.14. Catch, bullet #4. Upper-case ECMA. http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1184 src/runtime.cc:1184: // TODO(mmaly): Runtime_DeclareContextSlot. Needs strict mode? I don't think the declaration needs it. http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1397 src/runtime.cc:1397: // TODO(mmaly): Strict mode not needed (const disallowed). Agree. Remove TODOs (but retain rest of comment). http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1680 src/runtime.cc:1680: // No need for strict mode here. Called from SetupArray (array.js). Actually, if we make our builtins code strict, we should probably pass true here. Try using strict, and see if it works anyway. http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode3808 src/runtime.cc:3808: // TODO(mmaly): Implement SetElement strict mode. Make an entry in the v8 bug-tracker for the feature and use the issue number instead of your name in the TODO. http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc File src/stub-cache.cc (right): http://codereview.chromium.org/6594037/diff/1/src/stub-cache.cc#newcode1286 src/stub-cache.cc:1286: // Leaving JavaScript. TODO(mmaly): Do we need strict mode here? No. The callback is C++-code. It's not strict or non-strict, that only refers to JS code. If we ever need a callback that needs to know whether it's called from strict mode or not, we'll need to find a way to tell it, but currently we don't have one. http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc File src/x64/ic-x64.cc (right): http://codereview.chromium.org/6594037/diff/1/src/x64/ic-x64.cc#newcode1597 src/x64/ic-x64.cc:1597: void StoreIC::GenerateGlobalProxy(MacroAssembler* masm, StrictModeFlag strict) { Do! :) http://codereview.chromium.org/6594037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
