Second round of comments
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 { could we add a few lines about the modes, what they mean and why we have them 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)); As on arm, I actually think that the original comments gives value 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 || add comment here stating that the allowed transitions are: CLASSIC_MODE -> STRICT_MODE CLASSIC_MODE -> EXTENDED_MODE STRICT_MODE -> EXTENDED_MODE 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. 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) http://codereview.chromium.org/8417035/diff/29001/src/objects.h#newcode4977 src/objects.h:4977: inline bool is_extended_mode(); 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 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()); How about just having the mode in the class, everything is better than a bool for an argument :-) 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: ) { move ") {" up 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) { indention 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. Update comment 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. same comment as on arm, http://codereview.chromium.org/8417035/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
