LGTM
Mostly just js style nits but also some ideas to improve the tests.
https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/908883002/diff/140001/src/arm/full-codegen-arm.cc#newcode266
src/arm/full-codegen-arm.cc:266:
IsSubclassConstructor(info->function()->kind())
Do we do the right thing in the following case?
class C {
constructor() {
assertEquals(C, new.target);
}
}
new C();
The function above is not a subclass constructor.
Maybe this is just a TODO?
Maybe add a function to SHF called, HasNewTarget()?
https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/908883002/diff/140001/src/scopes.cc#newcode326
src/scopes.cc:326: variables_.Declare(this,
ast_value_factory_->new_target_string(), VAR,
Shouldn't this be CONST?
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js
File test/mjsunit/harmony/classes-experimental.js (right):
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode32
test/mjsunit/harmony/classes-experimental.js:32: constructor(x,y) {
ws after commma
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode89
test/mjsunit/harmony/classes-experimental.js:89: super(tmp(),4);
Because left to right execution?
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode90
test/mjsunit/harmony/classes-experimental.js:90: } catch(e) { exn = e; }
ws after catch
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode91
test/mjsunit/harmony/classes-experimental.js:91: assertTrue(exn !==
null);
instanceof test would be better
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode127
test/mjsunit/harmony/classes-experimental.js:127: assertTrue(s.__proto__
=== Subclass.prototype);
assertSame here too
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode145
test/mjsunit/harmony/classes-experimental.js:145: let s_2 = new
Subclass2();
no underscores in js
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode151
test/mjsunit/harmony/classes-experimental.js:151: super(x+y);
ws around binops
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode185
test/mjsunit/harmony/classes-experimental.js:185: return new Object();
in general, prefer {} over new Object()
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode205
test/mjsunit/harmony/classes-experimental.js:205: let f =
Subclass.bind(new Object());
maybe
let obj = {};
let f = Subclass.bind(obj);
and assert that `this !== obj` in the constructor.
https://codereview.chromium.org/908883002/diff/140001/test/mjsunit/harmony/classes-experimental.js#newcode205
test/mjsunit/harmony/classes-experimental.js:205: let f =
Subclass.bind(new Object());
Bind a parameter too.
let f = Subclass.bind({}, 1);
https://codereview.chromium.org/908883002/
--
--
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.
