Support for super keyed loads where key is a name.

Support for super keyed loads where key is a name.

R=arv@chromium.org, ishell@chromium.org
BUG=v8:3330
LOG=N

Committed: https://code.google.com/p/v8/source/detail?r=24403

[Dmitry Lomov (no reviews), 2014-10-01 13:53:19]

PTAL, not platform ports yet.

[arv (Not doing code reviews), 2014-10-01 15:20:24]

https://codereview.chromium.org/622523004/diff/1/src/runtime/runtime-classes.cc
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/622523004/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:86: if (!key->IsName()) {
How can this happen?

Maybe change to

CONVERT_ARG_HANDLE_CHECKED(Name, name, 2);

Can you add tests for these cases?

https://codereview.chromium.org/622523004/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:91: if (name->AsArrayIndex(&index)) {
Same here. When does this happen?

https://codereview.chromium.org/622523004/diff/1/src/runtime/runtime-classes....
src/runtime/runtime-classes.cc:103: CONVERT_ARG_HANDLE_CHECKED(Name, name, 2);
Is this backwards? I thought keyed mean `super[key]` so key can be anything.

https://codereview.chromium.org/622523004/diff/1/test/mjsunit/harmony/super.js
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/622523004/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:46: this[derivedDataProperty] = "xxx";
consistency

https://codereview.chromium.org/622523004/diff/1/test/mjsunit/harmony/super.j...
test/mjsunit/harmony/super.js:74: 
Can you add tests that uses numeric keys?

Can you add test that uses an object that has a toString and make sure that it
is only called once.

And a test that throws in toString

[Dmitry Lomov (no reviews), 2014-10-01 15:47:28]

Sorry for the noise looks like a wrong patch got uploaded. 
Nevermind.

[Dmitry Lomov (no reviews), 2014-10-02 08:59:08]

PTAL. 
Patch is fixed, platform ports done, added tests as Erik suggested (thanks!)

[Igor Sheludko, 2014-10-02 09:32:06]

https://codereview.chromium.org/622523004/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:2750: //  - this (receiver) <-- LoadFromSuper will
pop here and below.
LoadKeyedFromSuper in the comment

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:98: if (!Runtime::ToName(isolate,
key).ToHandle(&name)) {
I think this should look like:
  ASSIGN_RETURN_ON_EXCEPTION(isolate, name, Runtime::ToName(isolate, key),
Object);
to correctly handle propagate potential exception from toString() and not
replace it with UnsupportedSuper.

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:102: if (name->AsArrayIndex(&index)) {
And this case should be handled before Runtime::ToName(). Check the other
ToName() usages.

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:466: MUST_USE_RESULT
Remove MUST_USE_RESULT as it is already in the .h file.

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:168: super[1];  // Indexed properties unsupported
yet.
This case seems to be checked in TestUnsupportedCases().

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:177: derived.testGetterWithToString();
And please add a test for the toString() returning "1".

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:638: assertThrows(function(){f2.toMethod(o)(15);},
ReferenceError);
f1?

[Igor Sheludko, 2014-10-02 09:42:02]

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:102: if (name->AsArrayIndex(&index)) {
On 2014/10/02 09:32:05, Igor Sheludko wrote:
> And this case should be handled before Runtime::ToName(). Check the other
> ToName() usages.

Indeed, I mixed up ToArrayIndex() and AsArrayIndex(). So, ignore this comment.

[Dmitry Lomov (no reviews), 2014-10-02 09:54:12]

Comments addressed.

https://codereview.chromium.org/622523004/diff/20001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:2750: //  - this (receiver) <-- LoadFromSuper will
pop here and below.
On 2014/10/02 09:32:05, Igor Sheludko wrote:
> LoadKeyedFromSuper in the comment

Done in all ports

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:98: if (!Runtime::ToName(isolate,
key).ToHandle(&name)) {
On 2014/10/02 09:32:05, Igor Sheludko wrote:
> I think this should look like:
>   ASSIGN_RETURN_ON_EXCEPTION(isolate, name, Runtime::ToName(isolate, key),
> Object);
> to correctly handle propagate potential exception from toString() and not
> replace it with UnsupportedSuper.

Exception is propagated correctly (there is a test for that!), but what you
suggest follows the pattern in other places - done.

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:102: if (name->AsArrayIndex(&index)) {
On 2014/10/02 09:42:01, Igor Sheludko wrote:
> On 2014/10/02 09:32:05, Igor Sheludko wrote:
> > And this case should be handled before Runtime::ToName(). Check the other
> > ToName() usages.
> 
> Indeed, I mixed up ToArrayIndex() and AsArrayIndex(). So, ignore this comment.

Yup, ToArrayIndex in other places is a fast path that we do not handle (yet)

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime.cc
File src/runtime/runtime.cc (right):

https://codereview.chromium.org/622523004/diff/20001/src/runtime/runtime.cc#n...
src/runtime/runtime.cc:466: MUST_USE_RESULT
On 2014/10/02 09:32:06, Igor Sheludko wrote:
> Remove MUST_USE_RESULT as it is already in the .h file.

Done.

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:168: super[1];  // Indexed properties unsupported
yet.
On 2014/10/02 09:32:06, Igor Sheludko wrote:
> This case seems to be checked in TestUnsupportedCases().

Will keep it here as well if you don't mind.

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:177: derived.testGetterWithToString();
On 2014/10/02 09:32:06, Igor Sheludko wrote:
> And please add a test for the toString() returning "1".

Done.

https://codereview.chromium.org/622523004/diff/40001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:638: assertThrows(function(){f2.toMethod(o)(15);},
ReferenceError);
On 2014/10/02 09:32:06, Igor Sheludko wrote:
> f1?

Done.

[Igor Sheludko, 2014-10-02 10:16:41]

lgtm

[Dmitry Lomov (no reviews), 2014-10-02 10:17:43]

Igor, thanks a lot for the review!
Erik, could you take a look as well, esp. at tests? Many thanks!

[arv (Not doing code reviews), 2014-10-02 16:54:25]

LGTM

https://codereview.chromium.org/622523004/diff/60001/src/runtime/runtime-clas...
File src/runtime/runtime-classes.cc (right):

https://codereview.chromium.org/622523004/diff/60001/src/runtime/runtime-clas...
src/runtime/runtime-classes.cc:102: return ThrowUnsupportedSuper(isolate);
Add a FIXME/tracking bug?

https://codereview.chromium.org/622523004/diff/60001/test/mjsunit/harmony/sup...
File test/mjsunit/harmony/super.js (right):

https://codereview.chromium.org/622523004/diff/60001/test/mjsunit/harmony/sup...
test/mjsunit/harmony/super.js:46: this[derivedDataProperty] = "xxx";
still some style nits here

[Dmitry Lomov (no reviews), 2014-10-06 08:25:43]

Committed patchset #4 (id:60001) manually as 24403 (presubmit successful).