C++ LGTM
Tests could use some cleanup/improvements.
https://codereview.chromium.org/593073002/diff/80001/src/full-codegen.h
File src/full-codegen.h (right):
https://codereview.chromium.org/593073002/diff/80001/src/full-codegen.h#newcode524
src/full-codegen.h:524: // Load a value from super.named prroperty.
typo
https://codereview.chromium.org/593073002/diff/80001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):
https://codereview.chromium.org/593073002/diff/80001/src/runtime/runtime.cc#newcode2014
src/runtime/runtime.cc:2014:
RUNTIME_FUNCTION(Runtime_StoreToSuper_Sloppy) {
Why not take strict mode as a parameter?
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js
File test/mjsunit/harmony/super.js (right):
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode123
test/mjsunit/harmony/super.js:123: var getCalled = false;
Tip: These kinds of tests are always better written using a counter.
That way you also assert number of calls.
var getCalls = 0;
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode129
test/mjsunit/harmony/super.js:129: getCalled = true;
getCalls++;
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode144
test/mjsunit/harmony/super.js:144: assertTrue(42 == this);
If this is sloppy, should we not get a wrapper here? Maybe that is why
you are doing == which coerces?
Maybe do something like this to make it clearer?
assertEquals('object', typeof this);
assertTrue(this instanceof Number);
assertEquals(42, this.valueOf());
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode145
test/mjsunit/harmony/super.js:145: getCalled = false;
getCalls = 0;
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode148
test/mjsunit/harmony/super.js:148: assertTrue(getCalled);
assertEquals(1, getCalls);
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode166
test/mjsunit/harmony/super.js:166: assertTrue(42 == this);
=== and/or add typeof assert.
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode194
test/mjsunit/harmony/super.js:194: assertTrue(42 == this);
here as well. == is just too lenient to use for these kind of tests.
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode196
test/mjsunit/harmony/super.js:196: var except = false;
assertThrows cannot be used here but the following pattern is generally
better:
var ex;
try {
...
} catch (e) {
ex = e;
}
assertTrue(ex instanceof TypeError);
assertEquals('...', ex.message);
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode199
test/mjsunit/harmony/super.js:199: } catch(e) { except = true; }
ws after catch (like if, for, while, etc)
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode216
test/mjsunit/harmony/super.js:216: "use strict";
Don't mix and match quotes. Stick to '
https://codereview.chromium.org/593073002/diff/80001/test/mjsunit/harmony/super.js#newcode230
test/mjsunit/harmony/super.js:230: var o = {}
;
https://codereview.chromium.org/593073002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
