Addressed comments, please re-review.
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. Premature optimization (or complexification, as it may be). It does turn out that the same value works for all platforms (and it should, it's the same logical information that is stored). There are no platform-specific header files, and I wanted to avoid a #ifdef-mess in fast-codegen.h. On second thought, it seems the Finally and ForIn environment classes would be a better location for the values, especially if the code to remove the stack elements are moved to those classes as well. 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))); Just trying to avoid overflowing in the stack_elements * kPointerSize below. However, the argument size becomes ridiculous much earlier (the size of the stack is likely to be 2MB at most). I'll just remove the assert. There are so many ways to shoot yourself in the foot using assembler, if you don't know what you are doing, that one very permissive test won't make a difference. 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: Will remove blank line. http://codereview.chromium.org/466033/diff/1/6#newcode235 src/fast-codegen.cc:235: Yes, automatic unlinking will require the destructor to know codegen (or at least codegen.environment_). I was thinking something along the lines of Somethong(FastCodeGenerator codegen) : codegen_(codegen) { codegen->PushNestedStatement(this); } ~Something() { codegen_->PopNestedStatement(); } (or inline the Push/Pop in constructor/destructor, it's max three lines). http://codereview.chromium.org/466033/diff/1/6#newcode238 src/fast-codegen.cc:238: BreakTargetEnvironment break_env(stmt, &end_of_block); I agree, except for my dislike for doing the linking before the object is fully constructed (which will happen if is done in the superclass constructor. Is that even safe to do? The VTable hasn't been set up correctly yet at that point). Then again, we are not multithreaded. http://codereview.chromium.org/466033/diff/1/6#newcode245 src/fast-codegen.cc:245: environment_.Drop(1); Exactly. http://codereview.chromium.org/466033/diff/1/6#newcode307 src/fast-codegen.cc:307: __ Call(current->AsTryFinally()->finally_entry()); We have call(Label*) already in the ia32/x64 assemblers and bl(Label*) in the ARM assembler, so the functionality isn't new. It is true that using it uncritically is dangerous (but this is machine code, so doing anything uncritically is dangerous). The callee needs to know how to handle the pointer on the stack, either by hooking into stack traversal and GC (like the try-handler stack), and in this case finally blocks cook the address on entry. We can't, currently, call to a label in the old codegen, since we don't mix codegens inside a function. If we start doing that, we can't uncritically assume anything about labels from the other codegen (in either direction), and we should only jump between them at controlled points. http://codereview.chromium.org/466033/diff/1/6#newcode326 src/fast-codegen.cc:326: int target_offset = environment_.FindContinueTargetOffset(stmt->target()); It was an attempt to avoid duplicating the loop code (with only a difference in the test), but I agree that this isn't optimal either. I'll try avoiding the int. http://codereview.chromium.org/466033/diff/1/6#newcode333 src/fast-codegen.cc:333: int target_offset = environment_.FindBreakTargetOffset(stmt->target()); I think it should be ok. The current implementation also shadow all outer labels for each nesting, so a deeply nested statement with all nesting blocks labeled will also cause quadratic behavior. In practical code, it probably won't be measurable. We can make it even better by marking statements that are actually targets of breaks or continues, and only pushing the relevant ones in our nesting stack. 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. Agree that naming is bad. We are capturing the nesting of statements (but only the ones that we care about). It is, in a sense, the static evaluation context of the current statement. However, "context" is such an (over)loaded word too. How about an abstract superclass called NestedStatement, with Break, Continue, etc as sub-classes? I will move them into the code generator. http://codereview.chromium.org/466033/diff/1/7#newcode52 src/fast-codegen.h:52: class EnvironmentEntry { Will add BASE_EMBEDDED. http://codereview.chromium.org/466033/diff/1/7#newcode56 src/fast-codegen.h:56: virtual bool IsReturnTarget() { return false; } True, especially since AsXXX can be used directly in an if condition. http://codereview.chromium.org/466033/diff/1/7#newcode92 src/fast-codegen.h:92: ContinueTargetEnvironment(IterationStatement* continue_statement, No, but it is a continuation *target*. Still, with a rename of the superclass to NestedStatement, it should be named after the iteration statement, not its role as a continue target. It could even inherit from "Breakable". http://codereview.chromium.org/466033/diff/1/7#newcode162 src/fast-codegen.h:162: class ReturnTargetEnvironment : public EnvironmentEntry { Will do. http://codereview.chromium.org/466033/diff/1/7#newcode173 src/fast-codegen.h:173: class Environment { I wanted to make the environments linked too, but didn't find a combination of creating and linking that didn't seem messy. I should retry. Perhaps it looks better on the second try. Having the constructor link itself into a list is ... icky. Probably just a leftover from codeing Java, but I don't like pointers to a class to escape before the constructor is completely done. How about creating the "environemnt object" normally, and thne creating a RAII linker object on the side. I.e.: { TryFinally nested_finally; NestingLinker(&nested_finally); .. do something // Unlink of nested_finally. // Destruction of nested_finally. } http://codereview.chromium.org/466033/diff/1/7#newcode262 src/fast-codegen.h:262: environment_(), Does it hurt? Otherwise, I kindof like it there. http://codereview.chromium.org/466033/diff/1/7#newcode330 src/fast-codegen.h:330: void ExitEnvironments(int count); The latter. It emits code that cleans up the stack and calls finally blocks on the way, so that, at the end, you end up in a stack state that matches the expectations of the code at the nesting level you are exiting to. 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 would wonder too. Mostly about why the other platforms don't crash. http://codereview.chromium.org/466033/diff/1/11#newcode521 src/x64/fast-codegen-x64.cc:521: int return_offset = environment_.FindReturnOffset(); No, this is correct. The ExitEnvironments call will handle the finally blocks on the way out. http://codereview.chromium.org/466033 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
