[builtins] Move StringIndexOf to a C++ builtin.

[builtins] Move StringIndexOf to a C++ builtin.

BUG=v8:5364
Committed: https://crrev.com/58529ed3a05d086ed25047ddf170c69d1491f55e
Cr-Commit-Position: refs/heads/master@{#40093}

[petermarshall, 2016-09-20 13:44:06]

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

[commit-bot: I haz the power, 2016-09-20 13:44:13]

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-20 14:43:29]

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

[commit-bot: I haz the power, 2016-09-20 14:43:30]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-20 15:01:17]

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

[petermarshall, 2016-09-20 15:01:18]

I created a new CL for this, you can just ignore the old closed one with the
same title.

[Franzi, 2016-09-20 17:03:28]

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

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11604
src/objects.cc:11604: "String.prototype.lastIndexOf")));
Should this be String.prototpye.indexOf?

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11629
src/objects.cc:11629: int first_index = -1;
first_index is overwritten in any case, right? Can we inline it and call 
return Smi::FromInt(SearchString(...)) 
directly?

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11647
src/objects.cc:11647: if (seq_sub.IsOneByte()) {
Would it make sense to extract this if-else statement into a templated function
since it's the same as above? Or would that decrease readability?

https://codereview.chromium.org/2350963004/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2350963004/diff/1/src/objects.h#newcode9335
src/objects.h:9335: Handle<Object> pattern, Handle<Object> position);
Can we use the same parameter name for the third parameter as in LastIndexOf?

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

https://codereview.chromium.org/2350963004/diff/1/src/runtime/runtime-strings...
src/runtime/runtime-strings.cc:97: DCHECK(args.length() == 3);
Converting the previous code, this would be greater than or equal, not equal.
But we don't call  Runtime_StringReplaceOneCharWithString with a variable number
of arguments, so it should be OK.

[petermarshall, 2016-09-22 09:24:12]

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

[commit-bot: I haz the power, 2016-09-22 09:24:22]

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-22 09:56:24]

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

[commit-bot: I haz the power, 2016-09-22 09:56:27]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-09-22 13:54:25]

Have addressed Franzi's comments, PTAL

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

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11604
src/objects.cc:11604: "String.prototype.lastIndexOf")));
On 2016/09/20 at 17:03:27, Franzi wrote:
> Should this be String.prototpye.indexOf?

It definitely should

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11629
src/objects.cc:11629: int first_index = -1;
On 2016/09/20 at 17:03:27, Franzi wrote:
> first_index is overwritten in any case, right? Can we inline it and call 
> return Smi::FromInt(SearchString(...)) 
> directly?

Yep that's a bit nicer

https://codereview.chromium.org/2350963004/diff/1/src/objects.cc#newcode11647
src/objects.cc:11647: if (seq_sub.IsOneByte()) {
On 2016/09/20 at 17:03:27, Franzi wrote:
> Would it make sense to extract this if-else statement into a templated
function since it's the same as above? Or would that decrease readability?

Could be a good idea, I'm not completely sure of a nice way to do it, let's chat
offline about it.

https://codereview.chromium.org/2350963004/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2350963004/diff/1/src/objects.h#newcode9335
src/objects.h:9335: Handle<Object> pattern, Handle<Object> position);
On 2016/09/20 at 17:03:27, Franzi wrote:
> Can we use the same parameter name for the third parameter as in LastIndexOf?

Yep I'll do all of that renaming here too

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

https://codereview.chromium.org/2350963004/diff/1/src/runtime/runtime-strings...
src/runtime/runtime-strings.cc:97: DCHECK(args.length() == 3);
On 2016/09/20 at 17:03:28, Franzi wrote:
> Converting the previous code, this would be greater than or equal, not equal.
But we don't call  Runtime_StringReplaceOneCharWithString with a variable number
of arguments, so it should be OK.

The runtime functions need to have a fixed number of args anyway, mostly it
seems that the JS function handles the variable number of args and inserts a
default to the optional args when it calls the runtime function. So yeah,
calling this with 2 args would cause a compile error before the dcheck anyway.

[Benedikt Meurer, 2016-09-23 03:49:39]

Description was changed from

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

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

to

==========
[builtins] Move StringIndexOf to a C++ builtin.

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

[Benedikt Meurer, 2016-09-23 03:51:33]

https://codereview.chromium.org/2350963004/diff/20001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/2350963004/diff/20001/src/objects.cc#oldcode1...
src/objects.cc:11599: int start_index) {
I'd like to keep this version of String::IndexOf as-is, and add a wrapper around
it that does the error checking and stuff.

[petermarshall, 2016-09-23 09:34:02]

On 2016/09/23 at 03:51:33, bmeurer wrote:
> https://codereview.chromium.org/2350963004/diff/20001/src/objects.cc
> File src/objects.cc (left):
> 
>
https://codereview.chromium.org/2350963004/diff/20001/src/objects.cc#oldcode1...
> src/objects.cc:11599: int start_index) {
> I'd like to keep this version of String::IndexOf as-is, and add a wrapper
around it that does the error checking and stuff.

OK sounds good, I just ran into a problem converting StringIncludes that would
actually be helped by having a wrapper that does the checking. Plus we can avoid
the extra result-handling at other callsites of StringIndexOf. Not sure about
the naming, I've just overloaded String::IndexOf and added some comments in
objects.h. Open to other suggestions there.

[petermarshall, 2016-09-23 09:34:16]

PTAL

[Franzi, 2016-09-23 10:40:30]

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

[commit-bot: I haz the power, 2016-09-23 10:40:47]

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-23 10:49:13]

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

[commit-bot: I haz the power, 2016-09-23 10:49:14]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24846)

[Benedikt Meurer, 2016-09-23 11:03:11]

lgtm

[Franzi, 2016-09-23 11:37:00]

Hi Peter, 

I have a few minor nits. 

Cheers, 

Franzi

https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc#newcode1...
src/objects.cc:11617: double position_number = std::max(position->Number(),
0.0);
Can we call the double index and inline start_index?

https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc#newcode1...
src/objects.cc:11653: return first_index = SearchString(isolate,
receiver_content.ToUC16Vector(),
We can get rid of first_index.

https://codereview.chromium.org/2350963004/diff/60001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2350963004/diff/60001/src/objects.h#newcode9339
src/objects.h:9339: static int IndexOf(Isolate* isolate, Handle<String>
receiver,
Is the inner function only used inside Object* IndexOf()? Then I'd prefer a
helper function in an anonymous namespace.

[petermarshall, 2016-10-06 20:28:29]

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

[commit-bot: I haz the power, 2016-10-06 20:28:39]

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-10-06 20:30:05]

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

[commit-bot: I haz the power, 2016-10-06 20:30:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[petermarshall, 2016-10-06 21:33:28]

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

[commit-bot: I haz the power, 2016-10-06 21:33:34]

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-10-06 21:37:23]

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

[commit-bot: I haz the power, 2016-10-06 21:37:25]

Dry run: Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/25869)

[petermarshall, 2016-10-06 21:43:57]

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

[commit-bot: I haz the power, 2016-10-06 21:44:07]

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-10-06 22:17:09]

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

[commit-bot: I haz the power, 2016-10-06 22:17:10]

Dry run: This issue passed the CQ dry run.

[petermarshall, 2016-10-06 22:42:51]

On 2016/09/23 at 11:37:00, franzih wrote:
> Hi Peter, 
> 
> I have a few minor nits. 
> 
> Cheers, 
> 
> Franzi
> 
> https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc#newcode1...
> src/objects.cc:11617: double position_number = std::max(position->Number(),
0.0);
> Can we call the double index and inline start_index?
> 
>
https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc#newcode1...
> src/objects.cc:11653: return first_index = SearchString(isolate,
receiver_content.ToUC16Vector(),
> We can get rid of first_index.
> 
> https://codereview.chromium.org/2350963004/diff/60001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/2350963004/diff/60001/src/objects.h#newcode9339
> src/objects.h:9339: static int IndexOf(Isolate* isolate, Handle<String>
receiver,
> Is the inner function only used inside Object* IndexOf()? Then I'd prefer a
helper function in an anonymous namespace.
It has other callsites at the moment. But I did put the SearchString
vector-juggling helper into an anon namespace.

Cleaned up the rest of the code per you comments. Thanks

[petermarshall, 2016-10-06 22:43:11]

Franzi PTAL :)

https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2350963004/diff/60001/src/objects.cc#newcode1...
src/objects.cc:11617: double position_number = std::max(position->Number(),
0.0);
On 2016/09/23 at 11:37:00, Franzi wrote:
> Can we call the double index and inline start_index?

Done

[Benedikt Meurer, 2016-10-07 03:21:59]

LGTM.

[Franzi, 2016-10-07 07:29:16]

On 2016/10/07 at 03:21:59, bmeurer wrote:
> LGTM.

Thanks! LGTM.

[petermarshall, 2016-10-07 17:01:53]

The CQ bit was checked by petermarshall@chromium.org

[commit-bot: I haz the power, 2016-10-07 17:02:00]

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-10-07 17:04:00]

Description was changed from

==========
[builtins] Move StringIndexOf to a C++ builtin.

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

to

==========
[builtins] Move StringIndexOf to a C++ builtin.

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

[commit-bot: I haz the power, 2016-10-07 17:04:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-07 17:04:22]

Description was changed from

==========
[builtins] Move StringIndexOf to a C++ builtin.

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

to

==========
[builtins] Move StringIndexOf to a C++ builtin.

BUG=v8:5364

Committed: https://crrev.com/58529ed3a05d086ed25047ddf170c69d1491f55e
Cr-Commit-Position: refs/heads/master@{#40093}
==========

[commit-bot: I haz the power, 2016-10-07 17:04:24]

Patchset 8 (id:??) landed as
https://crrev.com/58529ed3a05d086ed25047ddf170c69d1491f55e
Cr-Commit-Position: refs/heads/master@{#40093}