I've got just a few comments on this.
http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.cc File src/ia32/lithium-ia32.cc (right): http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.cc#newcode89 src/ia32/lithium-ia32.cc:89: stream->Add(" = "); This adds a space before the '=' where before there was none. Is that intentional? http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (left): http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#oldcode343 src/ia32/lithium-ia32.h:343: T at(int i) const { return elems_[i]; } This seems like a place that you could legitimately have: T& operator[](int i) { return elems_[i]; } and get rid of both at and set_at. You could also assert that i is in bounds. http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode307 src/ia32/lithium-ia32.h:307: virtual void SetBranchTargets(int true_block_id, int false_block_id) { } Does this need to be on LInstruction? I would expect an LControlInstruction superclass of LUnaryControlInstruction and LBinaryControlInstruction where this belongs (and is non-virtual). http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode372 src/ia32/lithium-ia32.h:372: int InputCount() const { return inputs_.length(); } "return I" seems more direct. http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode376 src/ia32/lithium-ia32.h:376: int TempCount() const { return temps_.length(); } "return T"? http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode383 src/ia32/lithium-ia32.h:383: private: It also seems legitimate to make these protected so the subclasses can write: inputs_[0] = input rather than this->SetInputAt(0, input). http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode384 src/ia32/lithium-ia32.h:384: OperandContainer<LOperand*, R> outputs_; Perhaps outputs_ should be results_ (or R should be O, which is worse because it looks like 0) to keep up the symmetry. http://codereview.chromium.org/6237002/diff/1/src/ia32/lithium-ia32.h#newcode529 src/ia32/lithium-ia32.h:529: template<int R, int T = 0> We should strongly consider getting rid of classes LUnaryOperation and LBinaryOperation. There doesn't seem much reason to use inheritance for the number of inputs any more. Here, we're not getting much functionality (aliases left() and right() for inputs_[0] and inputs_[1], and a constructor so we can write LBinaryOperation(left, right) instead of inputs_[0] = left; inputs_[1] = right; in the subclasses. I'd like to get rid of them because it's confusing to sometimes have one, two, or three template parameters. It might be more clear to always have <R,I> with a default T and sometimes <R,I,T>. http://codereview.chromium.org/6237002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
