Unify ToArrayLength conversions

Unify ToArrayLength conversions

Committed: https://crrev.com/3c1f62ad3915b063e05d2de712886e723921c840
Cr-Commit-Position: refs/heads/master@{#31310}

[Jakob Kummerow, 2015-10-15 12:41:08]

jkummerow@chromium.org changed reviewers:
+ cbruni@chromium.org

[Jakob Kummerow, 2015-10-15 12:41:09]

PTAL. Just removing duplication of logic, no change in behavior intended.

[Toon Verwaest, 2015-10-15 12:58:21]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2015-10-15 12:58:22]

lgtm with comment

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6404
src/objects.cc:6404: if (length_object->ToUint32(output)) return true;
Perhaps change to ToArrayLength

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6429
src/objects.cc:6429: return uint32_v->ToArrayLength(output);
This should succeed afaict. Can we turn this into

CHECK(uint32_v->ToArrayLength(output));
return true;

to be explicit?

[Camillo Bruni, 2015-10-15 13:36:13]

cbruni@chromium.org changed reviewers:
- verwaest@chromium.org

[Camillo Bruni, 2015-10-15 13:36:13]

lgtm

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode718
src/objects.cc:718: if (num < 0) return false;
yes please :)

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6406
src/objects.cc:6406: Handle<String>::cast(length_object)->AsArrayIndex(output))
{
This case is almost handled in Object::ToUint32 => Object::ToNumber =>
String::ToNumber +/- some roundtrips to Double and Heapnumber :/

[Jakob Kummerow, 2015-10-15 14:12:06]

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode718
src/objects.cc:718: if (num < 0) return false;
On 2015/10/15 13:36:13, cbruni wrote:
> yes please :)

You're most welcome.

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6404
src/objects.cc:6404: if (length_object->ToUint32(output)) return true;
On 2015/10/15 12:58:22, Toon Verwaest wrote:
> Perhaps change to ToArrayLength

Done.

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6406
src/objects.cc:6406: Handle<String>::cast(length_object)->AsArrayIndex(output))
{
On 2015/10/15 13:36:13, cbruni wrote:
> This case is almost handled in Object::ToUint32 => Object::ToNumber =>
> String::ToNumber +/- some roundtrips to Double and Heapnumber :/

Yes -- this is the fast path; everything it handles is handled again by the slow
path below, but slower :-)

https://codereview.chromium.org/1404283002/diff/1/src/objects.cc#newcode6429
src/objects.cc:6429: return uint32_v->ToArrayLength(output);
On 2015/10/15 12:58:22, Toon Verwaest wrote:
> This should succeed afaict. Can we turn this into
> 
> CHECK(uint32_v->ToArrayLength(output));
> return true;
> 
> to be explicit?

Done.

[Jakob Kummerow, 2015-10-15 14:12:14]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-10-15 14:12:14]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/1404283002/#ps20001
(title: "addressed comments")

[commit-bot: I haz the power, 2015-10-15 14:12:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1404283002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1404283002/20001

[commit-bot: I haz the power, 2015-10-15 16:03:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-15 16:03:46]

Patchset 2 (id:??) landed as
https://crrev.com/3c1f62ad3915b063e05d2de712886e723921c840
Cr-Commit-Position: refs/heads/master@{#31310}