Add LookupIterator constructor for arbitrary Object keys

Add LookupIterator constructor for arbitrary Object keys

Continuing unification of properties/elements handling, the new
LookupIterator::PropertyOrElement(..., Handle<Object> key, ...) takes
any Object and does the required ToPrimitive/ToName/ToArrayIndex
conversions on it.

Committed: https://crrev.com/e3833fdce29a3726720ad2255475c99f6e50734f
Cr-Commit-Position: refs/heads/master@{#31023}

[Jakob Kummerow, 2015-09-29 14:40:01]

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

[Jakob Kummerow, 2015-09-29 14:40:01]

As discussed, PTAL.

[Toon Verwaest, 2015-09-30 13:02:34]

lgtm with comment.
I added a larger style suggestion, but up to you...

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode19
src/lookup.cc:19: Handle<Object> name,
What about calling this key?

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode24
src/lookup.cc:24: if (name->IsNumber()) {
What about a slightly less branchy version:

uint32_t index = 0;

if (key->ToArrayIndex(&index)) {
  *success = true;
  return LookupIterator(isolate, receiver, index, configuration);
}

Handle<Name> name;
*success = Object::ToName(isolate, key).ToHandle(&name);
if (!*success) {
  DCHECK(isolate->has_pending_exception());
  // Return an unusable dummy.
  return LookupIterator(receiver, isolate->factory()->empty_string());
}

if (name->AsArrayIndex(&index)) {
  LookupIterator it(isolate, receiver, index, configuration);
  // Here we try to avoid having to rebuild the string later
  // by storing it on the indexed LookupIterator.
  it.name_ = name;
  return it;
}

return LookupIterator(receiver, name, configuration);

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode47
src/lookup.cc:47: it.name_ = Handle<Name>::cast(name);
You don't need to set the .name_ if you know it's not an index (and you used
this particular constructor). The only reason the other static constructors do
it is to not have to convert index back to a name when needed for code that
needs the "original string version" of the property name.

[Jakob Kummerow, 2015-09-30 14:27:32]

Thanks for the review, I liked your suggestions. Landing.

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc
File src/lookup.cc (right):

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode19
src/lookup.cc:19: Handle<Object> name,
On 2015/09/30 13:02:33, Toon Verwaest wrote:
> What about calling this key?

Done.

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode24
src/lookup.cc:24: if (name->IsNumber()) {
On 2015/09/30 13:02:34, Toon Verwaest wrote:
> What about a slightly less branchy version:
> 
> uint32_t index = 0;
> 
> if (key->ToArrayIndex(&index)) {
>   *success = true;
>   return LookupIterator(isolate, receiver, index, configuration);
> }
> 
> Handle<Name> name;
> *success = Object::ToName(isolate, key).ToHandle(&name);
> if (!*success) {
>   DCHECK(isolate->has_pending_exception());
>   // Return an unusable dummy.
>   return LookupIterator(receiver, isolate->factory()->empty_string());
> }
> 
> if (name->AsArrayIndex(&index)) {
>   LookupIterator it(isolate, receiver, index, configuration);
>   // Here we try to avoid having to rebuild the string later
>   // by storing it on the indexed LookupIterator.
>   it.name_ = name;
>   return it;
> }
> 
> return LookupIterator(receiver, name, configuration);

Nice simplification. Done.

https://codereview.chromium.org/1375943002/diff/1/src/lookup.cc#newcode47
src/lookup.cc:47: it.name_ = Handle<Name>::cast(name);
On 2015/09/30 13:02:33, Toon Verwaest wrote:
> You don't need to set the .name_ if you know it's not an index (and you used
> this particular constructor). The only reason the other static constructors do
> it is to not have to convert index back to a name when needed for code that
> needs the "original string version" of the property name.

Done.
(Not sure why that was there... probably an artifact from an earlier version of
the code.)

[Jakob Kummerow, 2015-09-30 14:27:38]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2015-09-30 14:27:38]

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

[commit-bot: I haz the power, 2015-09-30 14:27:51]

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

[commit-bot: I haz the power, 2015-09-30 14:51:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-30 14:52:09]

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