Thanks for review, addressed comments
https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js
File src/harmony-classes.js (right):
https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#newcode15
src/harmony-classes.js:15: [%ToString(this), typeof this]);
On 2014/08/15 22:28:47, arv wrote:
Maybe TO_STRING_INLINE?
This is an error case, fine without inlining.
https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#newcode19
src/harmony-classes.js:19: if (!IS_SPEC_OBJECT(homeObject)) {
On 2014/08/15 22:28:47, arv wrote:
This is wrong order. This test should come before the is function
test.
Good catch. This must be a spec bug. Will file.
https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#newcode20
src/harmony-classes.js:20: throw MakeTypeError('toMethod_non_object');
On 2014/08/15 22:28:47, arv wrote:
Maybe include homeObject in the error message?
Done.
https://codereview.chromium.org/475423003/diff/20001/src/harmony-classes.js#newcode29
src/harmony-classes.js:29: "toMethod", FunctionToMethod
On 2014/08/15 22:28:47, arv wrote:
wrong indentation
Done.
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toMethod.js
File test/mjsunit/harmony/toMethod.js (right):
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toMethod.js#newcode31
test/mjsunit/harmony/toMethod.js:31: (function () {
On 2014/08/15 22:28:48, arv wrote:
Please name these. Makes it easier to know what you are testing
(function TestSomething() {
...
})();
Done.
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toMethod.js#newcode64
test/mjsunit/harmony/toMethod.js:64:
On 2014/08/15 22:28:48, arv wrote:
two new lines between top level functions
Done.
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toMethod.js#newcode80
test/mjsunit/harmony/toMethod.js:80: assertEquals(o,
fMeth[%HomeObjectSymbol()]);
On 2014/08/15 22:28:48, arv wrote:
Maybe define the private symbol in harmony-classes.js. That way there
is no need
for the runtime function.
I am not sure what the suggestion is.
a) we need HomeObjectSymbol outside of harmony-classes.js for codegen
b) without runtime function, how would we test this here?
https://codereview.chromium.org/475423003/diff/20001/test/mjsunit/harmony/toMethod.js#newcode104
test/mjsunit/harmony/toMethod.js:104: }());
On 2014/08/15 22:28:48, arv wrote:
Can you add a test that we check the type of the newHome in toMethod
before we
test the type of this.
I believe this is a spec bug.
Anyway, frankly not sure what is the good way to test the order of
checks - ideas?
https://codereview.chromium.org/475423003/
--
--
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.
