I addressed your comments from both rounds. Please take another look.
http://codereview.chromium.org/8417035/diff/29001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/arm/full-codegen-arm.cc#newcode2196
src/arm/full-codegen-arm.cc:2196: // Push the receiver of the enclosing
function.
On 2011/11/14 14:58:52, Rico wrote:
we still call runtime!
Done.
http://codereview.chromium.org/8417035/diff/29001/src/arm/full-codegen-arm.cc#newcode2200
src/arm/full-codegen-arm.cc:2200: // Push the language mode.
The previously implemented behavior does not correspond to what's in the
draft spec and was put in there because of false assumptions that
Andreas and I had. In ES.next the same happens as in ES5, the eval code
will be in the same mode unless it starts with a declarative that
changes it. So the comment is no true.
On 2011/11/14 14:58:52, Rico wrote:
Actually, I also sort of like the existing comment here, it is useful
and still
true, just change strict mode flat to language mode.
http://codereview.chromium.org/8417035/diff/29001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/arm/lithium-codegen-arm.cc#newcode2292
src/arm/lithium-codegen-arm.cc:2292: Handle<Code> ic =
(instr->strict_mode_flag() == kStrictMode)
The extended mode is also represented by the kStrictMode value in the
StrictModeFlag enum. And it correctly should share the IC with the
strict mode. However, your question strongly suggests that kStrictMode
should be renamed to avoid confusion. I already planned that, but want
to leave it to a renaming only CL.
On 2011/11/14 14:58:52, Rico wrote:
Can't we arrive here with extended mode flag? if so, should this be:
Handle<Code> ic = (instr->strict_mode_flag() == kNonStrictMode)
? isolate()->builtins()->StoreIC_Initialize()
? isolate()->builtins()->StoreIC_Initialize_Strict();
Same question for several occurrences below
http://codereview.chromium.org/8417035/diff/29001/src/compiler.cc
File src/compiler.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/compiler.cc#newcode606
src/compiler.cc:606: // After parsing we know function's language mode.
Remember it.
On 2011/11/14 14:58:52, Rico wrote:
we know the functions's ...
^
Done.
http://codereview.chromium.org/8417035/diff/29001/src/globals.h
File src/globals.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/globals.h#newcode364
src/globals.h:364: enum LanguageMode {
On 2011/11/15 08:25:07, Rico wrote:
could we add a few lines about the modes, what they mean and why we
have them
Done.
http://codereview.chromium.org/8417035/diff/29001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/ia32/full-codegen-ia32.cc#newcode2148
src/ia32/full-codegen-ia32.cc:2148: // Push the language mode.
On 2011/11/14 14:58:52, Rico wrote:
same comment as on arm
Done.
http://codereview.chromium.org/8417035/diff/29001/src/mips/full-codegen-mips.cc
File src/mips/full-codegen-mips.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/mips/full-codegen-mips.cc#newcode2267
src/mips/full-codegen-mips.cc:2267: __ lw(a1, MemOperand(fp,
receiver_offset * kPointerSize));
On 2011/11/15 08:25:07, Rico wrote:
As on arm, I actually think that the original comments gives value
Done.
http://codereview.chromium.org/8417035/diff/29001/src/objects-inl.h
File src/objects-inl.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/objects-inl.h#newcode3495
src/objects-inl.h:3495: ASSERT(this->language_mode() == CLASSIC_MODE ||
On 2011/11/15 08:25:07, Rico wrote:
add comment here stating that the allowed transitions are:
CLASSIC_MODE -> STRICT_MODE
CLASSIC_MODE -> EXTENDED_MODE
STRICT_MODE -> EXTENDED_MODE
Done.
http://codereview.chromium.org/8417035/diff/29001/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/objects.h#newcode4973
src/objects.h:4973: // Indicates whether the function is a classic mode
function.
On 2011/11/15 08:25:07, Rico wrote:
I would add "(i.e., classic in the sense that we have non-strict
semantics)"
after classic in this comment. V8 team members might not actually know
what
classic means yet, so a clarification here would be good)
Done.
http://codereview.chromium.org/8417035/diff/29001/src/objects.h#newcode4977
src/objects.h:4977: inline bool is_extended_mode();
On 2011/11/15 08:25:07, Rico wrote:
Also, please add a comment here (or below for language_mode())
explaining the
meaning of the 3 modes, I know this is in the commit message, but it
is
cumbersome to go there. Please also do this in globals.h
Done.
http://codereview.chromium.org/8417035/diff/29001/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/parser.cc#newcode3716
src/parser.cc:3716: this, !top_scope_->is_classic_mode());
On 2011/11/15 08:25:07, Rico wrote:
How about just having the mode in the class, everything is better than
a bool
for an argument :-)
Done.
http://codereview.chromium.org/8417035/diff/29001/src/preparse-data.h
File src/preparse-data.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/preparse-data.h#newcode93
src/preparse-data.h:93: ) {
On 2011/11/15 08:25:07, Rico wrote:
move ") {" up
Done.
http://codereview.chromium.org/8417035/diff/29001/src/preparser.h
File src/preparser.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/preparser.h#newcode421
src/preparser.h:421: : i::CLASSIC_MODE) {
On 2011/11/15 08:25:07, Rico wrote:
indention
Done.
http://codereview.chromium.org/8417035/diff/29001/src/scopes.h
File src/scopes.h (right):
http://codereview.chromium.org/8417035/diff/29001/src/scopes.h#newcode432
src/scopes.h:432: // This scope is a strict mode scope.
On 2011/11/15 08:25:07, Rico wrote:
Update comment
Done.
http://codereview.chromium.org/8417035/diff/29001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
http://codereview.chromium.org/8417035/diff/29001/src/x64/full-codegen-x64.cc#newcode2093
src/x64/full-codegen-x64.cc:2093: // Push the language mode.
On 2011/11/15 08:25:07, Rico wrote:
same comment as on arm,
Done.
http://codereview.chromium.org/8417035/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
