Reviewers: Kevin Millikin, Lasse Reichstein, fschneider, Message: Design discussion: If Reference's constructor pushes the state on the stack, and GetValue and SetValue and TakeValue pop the state from the stack, then the destructor can just verify that the state is popped. I assume that since a Reference scope is contained in the compilation of an expression, the only way runtime control flow exits is through generating a JS exception, so the stack is cleaned up. This means the stack height must be cleaned up, regardless of the extra stuff put on by the Reference.
Compile-time control flow leaving a Reference scope (because of compile errors? out-of-memory? we are in an expression) will call the Reference destructor, so they must not skip the GetValue and SetValue effects if we are checking the state (number of pushed copies of state remaining) in debug modes. Let me know what you think of this design. The code is not ready for review, of course. http://codereview.chromium.org/487017/diff/1/2 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/487017/diff/1/2#newcode4435 src/x64/codegen-x64.cc:4435: PushFrameSlotAt(element_count() - 2); Oops: frame_->PushFrameSlotAt. But it might be private. http://codereview.chromium.org/487017/diff/1/3 File src/x64/codegen-x64.h (right): http://codereview.chromium.org/487017/diff/1/3#newcode58 src/x64/codegen-x64.h:58: I use this enum argument, rather than a boolean argument. If we needed one call site to call both variants, this wouldn't work. Description: Refactor Reference so that SetValue and GetValue pop the reference state. Please review this at http://codereview.chromium.org/487017 SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ Affected files: M src/x64/codegen-x64.h M src/x64/codegen-x64.cc Index: src/x64/codegen-x64.cc =================================================================== --- src/x64/codegen-x64.cc (revision 3451) +++ src/x64/codegen-x64.cc (working copy) @@ -4354,11 +4354,25 @@ // CodeGenerator implementation of variables, lookups, and stores. Reference::Reference(CodeGenerator* cgen, Expression* expression) - : cgen_(cgen), expression_(expression), type_(ILLEGAL) { + : cgen_(cgen), expression_(expression), + type_(ILLEGAL), is_compound_assignment_(false), + reference_state_(ONE_COPY_ON_STACK) { cgen->LoadReference(this); } +Reference::Reference(CodeGenerator* cgen, + Expression* expression, + CompoundAssignmentFlag compound_assignment_flag) + : cgen_(cgen), expression_(expression), + type_(ILLEGAL), is_compound_assignment_(true), + reference_state_(TWO_COPIES_ON_STACK) { + ASSERT_EQ(COMPOUND_ASSIGNMENT, compound_assignment_flag); + cgen->LoadReference(this); + cgen->DuplicateReference(this); +} + + Reference::~Reference() { cgen_->UnloadReference(this); } @@ -4414,6 +4428,16 @@ } +void CodeGenerator::DuplicateReference(Reference* ref) { + if (ref->size() == 1 ) { + frame_->Dup(); + } else if (ref->size() == 2) { + PushFrameSlotAt(element_count() - 2); + PushFrameSlotAt(element_count() - 2); + } +} + + void CodeGenerator::UnloadReference(Reference* ref) { // Pop a reference from the stack while preserving TOS. Comment cmnt(masm_, "[ UnloadReference"); Index: src/x64/codegen-x64.h =================================================================== --- src/x64/codegen-x64.h (revision 3451) +++ src/x64/codegen-x64.h (working copy) @@ -54,7 +54,14 @@ public: // The values of the types is important, see size(). enum Type { ILLEGAL = -1, SLOT = 0, NAMED = 1, KEYED = 2 }; + enum CompoundAssignmentFlag { COMPOUND_ASSIGNMENT }; + Reference(CodeGenerator* cgen, Expression* expression); + // Creates a reference used by compound assignment, with the + // reference state pushed on the stack twice. + Reference(CodeGenerator* cgen, + Expression* expression, + CompoundAssignmentFlag compound_assignment_flag); ~Reference(); Expression* expression() const { return expression_; } @@ -91,9 +98,18 @@ void SetValue(InitState init_state); private: + // A ReferenceState is only used for debugging checks, to verify that + // compound assignment references are used correctly. No non-debug code + // should use these values. + enum ReferenceState { TWO_COPIES_ON_STACK, + ONE_COPY_ON_STACK, + NO_COPIES_ON_STACK }; + CodeGenerator* cgen_; Expression* expression_; Type type_; + bool is_compound_assignment_; + ReferenceState reference_state_; }; @@ -382,6 +398,8 @@ // The following are used by class Reference. void LoadReference(Reference* ref); + // Push a second copy of the state loaded by LoadReference on the frame. + void DuplicateReference(Reference* ref); void UnloadReference(Reference* ref); static Operand ContextOperand(Register context, int index) { -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
