First round. One real comment in the deoptimizer and a bunch of nits. I just looked at ia32, but I assume my comments apply to all architectures.
https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h File src/builtins.h (right): https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h#newcode160 src/builtins.h:160: V(SetterStubForDeopt, STORE_IC, MONOMORPHIC, \ Can we call this "StoreIC_Setter_ForDeopt"? https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc File src/ia32/deoptimizer-ia32.cc (right): https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode700 src/ia32/deoptimizer-ia32.cc:700: // The receiver and RHS are expected in registers by the IC, so they don't s/IC/StoreIC/ https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode702 src/ia32/deoptimizer-ia32.cc:702: // of 0 instead of 2. Drop the "instead of 2" part. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode709 src/ia32/deoptimizer-ia32.cc:709: // 1 stack entry for the return address + 4 stack entries from Can we start this sentence with a word, not a number, to make it a proper sentence. Also we should really think about moving these constants in frames-[arch].h at some point, but that's for a followup CL. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode777 src/ia32/deoptimizer-ia32.cc:777: value = reinterpret_cast<intptr_t>(setter->code()); I don't think this is quite right. What you want here is the code object for the setter stub, not for the JavaScript setter. So this should be "setter_stub" instead. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode790 src/ia32/deoptimizer-ia32.cc:790: // The RHS was part of the artificial setter stub environment. I would use "implicit return value" or "passed value" here instead of RHS, because RHS only applies to ordinary assignments, it has little meaning for counting or compound assignments. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimizer-ia32.cc#newcode796 src/ia32/deoptimizer-ia32.cc:796: Code* setter_stub = Move this up, you already need if for the code object. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cache-ia32.cc File src/ia32/stub-cache-ia32.cc (right): https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cache-ia32.cc#newcode2669 src/ia32/stub-cache-ia32.cc:2669: if (setter.is_null()) { Can we flip the condition, thereby the return point would visually be after the InvokeFunction part, where it actually needs to be. That's visually more intuitive (at least for me). https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#newcode8307 src/objects.cc:8307: case Translation::SETTER_STUB_FRAME: { Can we make that an own case that doesn't print the height at all? This makes it hard to see when we expect the iterator to give us a height and when not. https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#newcode8312 src/objects.cc:8312: opcode == Translation::SETTER_STUB_FRAME ? 2 : iterator.Next(); Actually, height = 0 for the setter stub, but that doesn't matter if the above comment is addressed. https://chromiumcodereview.appspot.com/10855098/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
