[builtins] Move StringLastIndexOf to a builtin.

[builtins] Move StringLastIndexOf to a builtin.

BUG=v8:5364
Committed: https://crrev.com/95cc98c99ce95d061a7c27f9b6a45177173f0945
Cr-Commit-Position: refs/heads/master@{#39498}

[petermarshall, 2016-09-14 13:45:35]

The CQ bit was checked by petermarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 13:45:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 14:12:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 14:12:17]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-14 14:52:19]

Description was changed from

==========
[builtins] Move StringLastIndexOf to c++.

BUG=
==========

to

==========
[builtins] Move StringLastIndexOf to c++.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=
==========

[petermarshall, 2016-09-14 14:52:40]

Description was changed from

==========
[builtins] Move StringLastIndexOf to c++.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=
==========

to

==========
[builtins] Move StringLastIndexOf to a builtin.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=
==========

[petermarshall, 2016-09-14 14:53:21]

petermarshall@chromium.org changed reviewers:
+ bmeurer@chromium.org, franzih@chromium.org

[Franzi, 2016-09-14 15:05:01]

Very nice! Do we have an issue for that? Either v8:5364 or you can create one
and make it block v8:5049. Thanks.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:480: template <typename schar, typename pchar>
I think this should be inside an anonymous namespace.

https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:7457: -    "  return %StringLastIndexOf(x, 'bob', 10);"
Rather than deleting the test, can we use a different runtime function? From
what I understand we're trying to test that native runtime calls are supported
in extensions, it shouldn't matter which runtime function we call.

[Benedikt Meurer, 2016-09-14 17:02:32]

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:480: template <typename schar, typename pchar>
Yes, please remove the static and put it into anonymous namespace.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:529: position =
isolate->factory()->infinity_value();
You can just default to sub->length here, as infinity will be clamped to that
value anyways. and then you need to do the additional checks below only in the
else case.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:535: Handle<Object> zero =
isolate->factory()->NewNumberFromInt(0);
You don't need this boxed zero valid, see comments below.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:537: if (Object::LessThan(position,
zero).FromJust()) {
No need to use Object::LessThan here, which corresponds to the JavaScript <
operator. At this point position is known to be a number, so you can just write

  double position_number = std::max(position->Number(), 0.0);

and use position_number below.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:542:
isolate->factory()->NewNumberFromInt(sub->length());
No need to box the sub->length, just compare against the position_number.

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:548: if (!position->ToArrayIndex(&start_index))
return Smi::FromInt(-1);
At this point you know that position_number is within [0,max-string-length] and
max-string-length is less than kMaxUint32-1, so you can just write

  uint32_t start_index = static_cast<uint32_t>(position_number);

https://codereview.chromium.org/2339123002/diff/40001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2339123002/diff/40001/src/js/i18n.js#newcode317
src/js/i18n.js:317: var pos = locale.lastIndexOf('-');
This is not correct, because this does a lookup of lastIndexOf on locale each
time you executed it. But String.prototype.lastIndexOf is monkey patchable,
which means you might get a different function here. So you really need to
remember the correct function during bootstrapping. Or even better, put it onto
the native context and call it via %StringLastIndexOf(locale, '-') here, as we
do with other initial functions like %ObjectFreeze, etc.

https://codereview.chromium.org/2339123002/diff/40001/src/js/i18n.js#newcode440
src/js/i18n.js:440: var pos = locale.lastIndexOf('-');
Same here.

https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:7457: -    "  return %StringLastIndexOf(x, 'bob', 10);"
Yes, that's correct. Please keep the test.

[petermarshall, 2016-09-15 12:16:43]

On 2016/09/14 at 15:05:01, franzih wrote:
> Very nice! Do we have an issue for that? Either v8:5364 or you can create one
and make it block v8:5049. Thanks.
> 
>
https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
> File src/builtins/builtins-string.cc (right):
> 
>
https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
> src/builtins/builtins-string.cc:480: template <typename schar, typename pchar>
> I think this should be inside an anonymous namespace.
> 
> https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc
> File test/cctest/test-api.cc (left):
> 
>
https://codereview.chromium.org/2339123002/diff/40001/test/cctest/test-api.cc...
> test/cctest/test-api.cc:7457: -    "  return %StringLastIndexOf(x, 'bob',
10);"
> Rather than deleting the test, can we use a different runtime function? From
what I understand we're trying to test that native runtime calls are supported
in extensions, it shouldn't matter which runtime function we call.

Changed it to use StringCompare instead

[petermarshall, 2016-09-15 12:18:22]

Description was changed from

==========
[builtins] Move StringLastIndexOf to a builtin.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=
==========

to

==========
[builtins] Move StringLastIndexOf to a builtin.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=v8:5364
==========

[petermarshall, 2016-09-15 12:19:58]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-09-15 12:19:59]

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
File src/builtins/builtins-string.cc (right):

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:480: template <typename schar, typename pchar>
On 2016/09/14 at 15:05:01, Franzi wrote:
> I think this should be inside an anonymous namespace.

Done

https://codereview.chromium.org/2339123002/diff/40001/src/builtins/builtins-s...
src/builtins/builtins-string.cc:548: if (!position->ToArrayIndex(&start_index))
return Smi::FromInt(-1);
On 2016/09/14 at 17:02:32, Benedikt Meurer wrote:
> At this point you know that position_number is within [0,max-string-length]
and max-string-length is less than kMaxUint32-1, so you can just write
> 
>   uint32_t start_index = static_cast<uint32_t>(position_number);

I reworked this whole section with your comments

https://codereview.chromium.org/2339123002/diff/40001/src/js/i18n.js
File src/js/i18n.js (right):

https://codereview.chromium.org/2339123002/diff/40001/src/js/i18n.js#newcode317
src/js/i18n.js:317: var pos = locale.lastIndexOf('-');
On 2016/09/14 at 17:02:32, Benedikt Meurer wrote:
> This is not correct, because this does a lookup of lastIndexOf on locale each
time you executed it. But String.prototype.lastIndexOf is monkey patchable,
which means you might get a different function here. So you really need to
remember the correct function during bootstrapping. Or even better, put it onto
the native context and call it via %StringLastIndexOf(locale, '-') here, as we
do with other initial functions like %ObjectFreeze, etc.

Added the runtime function back in as we discussed

[commit-bot: I haz the power, 2016-09-15 12:20:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 12:20:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 12:20:06]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Franzi, 2016-09-15 15:37:18]

The CQ bit was checked by franzih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 15:37:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 16:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-15 16:07:01]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-16 09:51:50]

The CQ bit was checked by petermarshall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 09:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 09:54:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 09:54:29]

Dry run: This issue passed the CQ dry run.

[Franzi, 2016-09-19 08:04:22]

LGTM with nits.

https://codereview.chromium.org/2339123002/diff/80001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2339123002/diff/80001/src/objects.cc#newcode1...
src/objects.cc:11684: Handle<String> sub;
Can we use a better name than sub?

https://codereview.chromium.org/2339123002/diff/80001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2339123002/diff/80001/src/objects.h#newcode9256
src/objects.h:9256: 
I don't like pat as variable name (even thought the JS function was using the
same). Can we use search_string or string?

[petermarshall, 2016-09-19 08:33:56]

Description was changed from

==========
[builtins] Move StringLastIndexOf to a builtin.

Remove the cctest that tests for the lastIndexOf runtime function, as this is
entirely a builtin now.

BUG=v8:5364
==========

to

==========
[builtins] Move StringLastIndexOf to a builtin.

BUG=v8:5364
==========

[petermarshall, 2016-09-19 09:18:21]

On 2016/09/19 at 08:04:22, franzih wrote:
> LGTM with nits.
> 
> https://codereview.chromium.org/2339123002/diff/80001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2339123002/diff/80001/src/objects.cc#newcode1...
> src/objects.cc:11684: Handle<String> sub;
> Can we use a better name than sub?
> 
> https://codereview.chromium.org/2339123002/diff/80001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/2339123002/diff/80001/src/objects.h#newcode9256
> src/objects.h:9256: 
> I don't like pat as variable name (even thought the JS function was using the
same). Can we use search_string or string?

Thanks, those names were terrible

[petermarshall, 2016-09-19 09:21:09]

Benedikt, do you have any further suggestions?

[Benedikt Meurer, 2016-09-19 09:49:42]

LGTM, thanks.

[petermarshall, 2016-09-19 09:51:59]

The CQ bit was checked by petermarshall@chromium.org

[petermarshall, 2016-09-19 09:52:00]

The patchset sent to the CQ was uploaded after l-g-t-m from franzih@chromium.org
Link to the patchset: https://codereview.chromium.org/2339123002/#ps120001
(title: "Variable renaming to make things a bit clearer")

[commit-bot: I haz the power, 2016-09-19 09:52:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 10:22:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-19 10:23:02]

Description was changed from

==========
[builtins] Move StringLastIndexOf to a builtin.

BUG=v8:5364
==========

to

==========
[builtins] Move StringLastIndexOf to a builtin.

BUG=v8:5364

Committed: https://crrev.com/95cc98c99ce95d061a7c27f9b6a45177173f0945
Cr-Commit-Position: refs/heads/master@{#39498}
==========

[commit-bot: I haz the power, 2016-09-19 10:23:03]

Patchset 7 (id:??) landed as
https://crrev.com/95cc98c99ce95d061a7c27f9b6a45177173f0945
Cr-Commit-Position: refs/heads/master@{#39498}