Thanks for the review. I made the changes and am going ahead with the
commit.
Thank you!
Martin
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?
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
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.
Done.
http://codereview.chromium.org/6594037/diff/1/src/builtins.h
File src/builtins.h (right):
http://codereview.chromium.org/6594037/diff/1/src/builtins.h#newcode154
src/builtins.h:154: V(KeyedStoreIC_Initialize_Strict, KEYED_STORE_IC,
UNINITIALIZED, \
On 2011/02/28 12:23:06, Mads Ager wrote:
This is the reason why I don't like indentation like this. Either we
should
indent them all so they align or we should drop the alignment
altogether.
Done.
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);
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
You could combine the values into a "StoreICFlag" encoded value (e.g.,
using
BitFields), but this should be fine.
I will keep it as-is for now.
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
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Comment isn't necessary here. The variable is self-explanatory :).
Done.
http://codereview.chromium.org/6594037/diff/1/src/ia32/stub-cache-ia32.cc#newcode3720
src/ia32/stub-cache-ia32.cc:3720: : kNonStrictMode)));
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Is the conditional necessary here? It seems
Code::ExtractExtraICStateFromFlags(flags) returns kStrictMode directly
(or at
most something castable to it).
Stritly speaking, the extra state is 2 bits so I think the best thing is
to mask out the strict mode bit and push that directly. Done that.
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
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Put parentheses around any non-trivial condition of a ?: operator.
Done across the change.
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#newcode1485
src/ic.cc:1485: // Strict mode doesn't allow setting non-existent global
property.
On 2011/02/28 12:23:06, Mads Ager wrote:
Update comment to reflect the read-only part of this code.
Done.
http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1488
src/ic.cc:1488: return TypeError("strict_rdonly_property", object,
name);
On 2011/02/28 12:23:06, Mads Ager wrote:
Please don't abbreviate read to rd.
Done.
http://codereview.chromium.org/6594037/diff/1/src/ic.cc#newcode1947
src/ic.cc:1947: // ASSERT(extra_ic_state ==
ic.target()->extra_ic_state());
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Should be removed before commit.
Done.
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());
On 2011/02/28 11:35:05, Vitaly wrote:
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
> 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.
Please don't kill extra_ic_state. It's also used as an opaque extra
state to
gather more type feedback. But having another accessor that wraps it
is a
probably a good idea.
I am keeping the extra_ic_state but changing the condition to mask only
for the bit that this code cares about. This leaves the rest of the VM
to use the other bit.
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.
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Upper-case ECMA.
Done.
http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1184
src/runtime.cc:1184: // TODO(mmaly): Runtime_DeclareContextSlot. Needs
strict mode?
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
I don't think the declaration needs it.
Removed comment.
http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode1397
src/runtime.cc:1397: // TODO(mmaly): Strict mode not needed (const
disallowed).
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Agree. Remove TODOs (but retain rest of comment).
Done.
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).
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Actually, if we make our builtins code strict, we should probably pass
true
here.
Try using strict, and see if it works anyway.
I am not sure it really matters one way or another. the strict would
only result in an exception if the property already existed and was read
only. It won't affect in any way the attributes of the property or how
the function is later called. strict may be safer because if our
initialization code has bugs we get an exception rather than silently
ignoring the assignment.
http://codereview.chromium.org/6594037/diff/1/src/runtime.cc#newcode3808
src/runtime.cc:3808: // TODO(mmaly): Implement SetElement strict mode.
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Make an entry in the v8 bug-tracker for the feature and use the issue
number
instead of your name in the TODO.
Done. Issue 1220.
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) {
On 2011/02/28 11:18:30, Lasse Reichstein wrote:
Do! :)
Done across the board.
http://codereview.chromium.org/6594037/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
