In general this LGTM. Only a few nits to address.
https://chromiumcodereview.appspot.com/10388047/diff/1015/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10388047/diff/1015/src/hydrogen.cc#newcode4278 src/hydrogen.cc:4278: CHECK_ALIVE({}); Can we rewrite that to use the following syntax. I think that's more in sync with the rest of the code. HInstruction* store; CHECK_ALIVE(store = BuildStoreNamed(literal, value, property)); https://chromiumcodereview.appspot.com/10388047/diff/1015/src/hydrogen.cc#newcode4631 src/hydrogen.cc:4631: CHECK_ALIVE({}); Likewise. Also for the rest of the file. https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc#newcode1769 src/objects.cc:1769: result_object = SetPropertyViaPrototypes( The method call should fit into one line if we break right after the '=' sign. https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc#newcode2051 src/objects.cc:2051: void JSObject::LookupCallbackSetterInPrototypes(String* name, Can we rename this to "LookupRealNamedProperty" or something similar? At least the current name makes no sense anymore I think. https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc#newcode2134 src/objects.cc:2134: *done = !!(attr & READ_ONLY); Why not not not use no negation? https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc#newcode2150 src/objects.cc:2150: ; // Fall through Fall through to nothing makes no sense, can we just use "break;" here? https://chromiumcodereview.appspot.com/10388047/diff/1015/src/objects.cc#newcode2944 src/objects.cc:2944: MaybeObject* result_object = SetPropertyViaPrototypes( The method call should fit into one line if we break right after the '=' sign. https://chromiumcodereview.appspot.com/10388047/diff/1015/test/mjsunit/fuzz-accessors.js File test/mjsunit/fuzz-accessors.js (right): https://chromiumcodereview.appspot.com/10388047/diff/1015/test/mjsunit/fuzz-accessors.js#newcode69 test/mjsunit/fuzz-accessors.js:69: print("running"); I think we should just remove all of this debugging code again. https://chromiumcodereview.appspot.com/10388047/diff/1015/test/mjsunit/regress/regress-334.js File test/mjsunit/regress/regress-334.js (right): https://chromiumcodereview.appspot.com/10388047/diff/1015/test/mjsunit/regress/regress-334.js#newcode50 test/mjsunit/regress/regress-334.js:50: print(i); Can we remove this debugging code again. https://chromiumcodereview.appspot.com/10388047/diff/1015/test/test262/test262.status File test/test262/test262.status (left): https://chromiumcodereview.appspot.com/10388047/diff/1015/test/test262/test262.status#oldcode51 test/test262/test262.status:51: 15.2.3.6-4-420: FAIL I like that!!! https://chromiumcodereview.appspot.com/10388047/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
