As we discussed, we might try some variation of NestingStack or NestingEnvironment as a name.
http://codereview.chromium.org/466033/diff/1/2 File src/arm/fast-codegen-arm.cc (right): http://codereview.chromium.org/466033/diff/1/2#newcode41 src/arm/fast-codegen-arm.cc:41: // TODO(lrn) - set the constant when implementing finally. I don't understand the TODO. I don't understand why this constant is platform-dependent. Even if it has to be platform independent, it should be in the header file. http://codereview.chromium.org/466033/diff/1/3 File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/466033/diff/1/3#newcode166 src/arm/macro-assembler-arm.cc:166: ASSERT(stack_elements < (1 << (30 - kPointerSizeLog2))); I don't understand the purpose of the assert. It's too magic to not have a comment. http://codereview.chromium.org/466033/diff/1/6 File src/fast-codegen.cc (right): http://codereview.chromium.org/466033/diff/1/6#newcode235 src/fast-codegen.cc:235: This blank line is surely not necessary. http://codereview.chromium.org/466033/diff/1/6#newcode237 src/fast-codegen.cc:237: Likewise. http://codereview.chromium.org/466033/diff/1/6#newcode238 src/fast-codegen.cc:238: BreakTargetEnvironment break_env(stmt, &end_of_block); All these classes have the protocol (1) construct, immediately (2) push on environment_, bracketed by (3) pop. Perfect candidate for RAII so you just write: Break break_target(this, stmt, &end_of_block); VisitStatements(stmt->statements()); __ bind(&end_of_block); http://codereview.chromium.org/466033/diff/1/6#newcode241 src/fast-codegen.cc:241: SetStatementPosition(stmt); Not your code, but I wonder why we SetStatementPosition here. http://codereview.chromium.org/466033/diff/1/6#newcode326 src/fast-codegen.cc:326: int target_offset = environment_.FindContinueTargetOffset(stmt->target()); The int seems like it doesn't need to be exposed in the API. Consider combining into a single function that searches and exits. http://codereview.chromium.org/466033/diff/1/7 File src/fast-codegen.h (right): http://codereview.chromium.org/466033/diff/1/7#newcode42 src/fast-codegen.h:42: // Environments (or scopes) of special significance. Terminology throughout needs tweaking. These are not "environments" (they don't map anything to anything) and they're not "scopes" in any way connected to JS. They're all statements or blocks. Idea: just use "Break", "Continue", "TryCatch", "TryFinally", "Finally", "ForIn" and make them subclasses of the codegenerator to give yourself a namespace. http://codereview.chromium.org/466033/diff/1/7#newcode52 src/fast-codegen.h:52: class EnvironmentEntry { It seems strange that an "XXXEnvironment" is a subclass of "EnvironmentEntry". Also, BASE_EMBEDDED please. http://codereview.chromium.org/466033/diff/1/7#newcode56 src/fast-codegen.h:56: virtual bool IsReturnTarget() { return false; } Having both the IsXXX() and AsXXX style of cast operators seeems like overkill for such a simple utility class. http://codereview.chromium.org/466033/diff/1/7#newcode92 src/fast-codegen.h:92: ContinueTargetEnvironment(IterationStatement* continue_statement, Bad name. If it's an IterationStatement it's definitely not a continue statement. http://codereview.chromium.org/466033/diff/1/7#newcode162 src/fast-codegen.h:162: class ReturnTargetEnvironment : public EnvironmentEntry { I would make the return implicit. It's always and only the last element of the (reversed) list and has no fields. http://codereview.chromium.org/466033/diff/1/7#newcode173 src/fast-codegen.h:173: class Environment { Instead of a separate ZoneList of pointers to stack-allocated objects, I'd just thread the objects through the stack with a pointer in each. Probably just because I hate ZoneList. http://codereview.chromium.org/466033/diff/1/7#newcode262 src/fast-codegen.h:262: environment_(), No need. http://codereview.chromium.org/466033/diff/1/11 File src/x64/fast-codegen-x64.cc (right): http://codereview.chromium.org/466033/diff/1/11#newcode158 src/x64/fast-codegen-x64.cc:158: ReturnTargetEnvironment return_env; I wonder why the other platforms don't need this? http://codereview.chromium.org/466033 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
