LGTM (with a few nits).
https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc#newcode4723 src/hydrogen.cc:4723: (*accessors)->getter()->IsJSFunction()) { No need to do open the handle here, that will be done implicitly, just use ... accessors->getter()->IsJSFunction() https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc#newcode4724 src/hydrogen.cc:4724: *getter = Handle<JSFunction>(JSFunction::cast((*accessors)->getter())); Likewise. https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc#newcode4737 src/hydrogen.cc:4737: (*accessors)->setter()->IsJSFunction()) { Likewise. https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc#newcode4738 src/hydrogen.cc:4738: *setter = Handle<JSFunction>(JSFunction::cast((*accessors)->setter())); Likewise. https://chromiumcodereview.appspot.com/10825384/diff/1/src/hydrogen.cc#newcode7920 src/hydrogen.cc:7920: // Because we re-use the load type feedback, there might be no setter. We should remove this comment, because there might be no setter for other reasons as well. This is misleading. https://chromiumcodereview.appspot.com/10825384/diff/1/test/mjsunit/object-define-property.js File test/mjsunit/object-define-property.js (right): https://chromiumcodereview.appspot.com/10825384/diff/1/test/mjsunit/object-define-property.js#newcode1176 test/mjsunit/object-define-property.js:1176: // Test assignment to a getter-only property on the prototype chain. Can we extend this comment to say that it actually tests that Crankshaft doesn't solely rely on type-feedback but rechecks assumptions. https://chromiumcodereview.appspot.com/10825384/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
