[builtins] Move StringLastIndexOf to a builtin.

src/builtins/builtins-string.cc

Index: src/builtins/builtins-string.cc
diff --git a/src/builtins/builtins-string.cc b/src/builtins/builtins-string.cc
index f7413fc30ff6681293e0c8157c4fdc7b5f03e0c8..f95b60704be1b7e485f1a60fd3a1d312c833763f 100644
--- a/src/builtins/builtins-string.cc
+++ b/src/builtins/builtins-string.cc
@@ -477,6 +477,118 @@ void Builtins::Generate_StringPrototypeCharCodeAt(
   assembler->Return(result);
 }
 
+template <typename schar, typename pchar>

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

I think this should be inside an anonymous namespace. 

[Benedikt Meurer, 2016/09/14 17:02:31]

Yes, please remove the static and put it into anonymous namespace.

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

Done

+static int StringMatchBackwards(Vector<const schar> subject,
+                                Vector<const pchar> pattern, int idx) {
+  int pattern_length = pattern.length();
+  DCHECK(pattern_length >= 1);
+  DCHECK(idx + pattern_length <= subject.length());
+
+  if (sizeof(schar) == 1 && sizeof(pchar) > 1) {
+    for (int i = 0; i < pattern_length; i++) {
+      uc16 c = pattern[i];
+      if (c > String::kMaxOneByteCharCode) {
+        return -1;
+      }
+    }
+  }
+
+  pchar pattern_first_char = pattern[0];
+  for (int i = idx; i >= 0; i--) {
+    if (subject[i] != pattern_first_char) continue;
+    int j = 1;
+    while (j < pattern_length) {
+      if (pattern[j] != subject[i + j]) {
+        break;
+      }
+      j++;
+    }
+    if (j == pattern_length) {
+      return i;
+    }
+  }
+  return -1;
+}
+
+// ES6 section 21.1.3.9
+// String.prototype.lastIndexOf ( searchString [ , position ] )
+BUILTIN(StringPrototypeLastIndexOf) {
+  HandleScope handle_scope(isolate);
+  TO_THIS_STRING(sub, "String.prototype.lastIndexOf");
+
+  Handle<String> pat;
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, pat,
+      Object::ToString(isolate, args.atOrUndefined(isolate, 1)));
+
+  Handle<Object> position;
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, position, Object::ToNumber(args.atOrUndefined(isolate, 2)));
+
+  if (position->IsNaN()) {
+    position = isolate->factory()->infinity_value();

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

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.

+  } else {
+    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, position,
+                                       Object::ToInteger(isolate, position));
+  }
+
+  Handle<Object> zero = isolate->factory()->NewNumberFromInt(0);

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

You don't need this boxed zero valid, see comments below.

+
+  if (Object::LessThan(position, zero).FromJust()) {

[Benedikt Meurer, 2016/09/14 17:02:31]

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.

+    position = zero;
+  }
+
+  Handle<Object> sub_len_obj =
+      isolate->factory()->NewNumberFromInt(sub->length());

[Benedikt Meurer, 2016/09/14 17:02:31]

No need to box the sub->length, just compare against the position_number.

+  if (Object::LessThan(sub_len_obj, position).FromJust()) {
+    position = sub_len_obj;
+  }
+
+  uint32_t start_index;
+  if (!position->ToArrayIndex(&start_index)) return Smi::FromInt(-1);

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

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);

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

I reworked this whole section with your comments

+
+  uint32_t pat_length = pat->length();
+  uint32_t sub_length = sub->length();
+
+  if (start_index + pat_length > sub_length) {
+    start_index = sub_length - pat_length;
+  }
+
+  if (pat_length == 0) {
+    return Smi::FromInt(start_index);
+  }
+
+  sub = String::Flatten(sub);
+  pat = String::Flatten(pat);
+
+  int last_index = -1;
+  DisallowHeapAllocation no_gc;  // ensure vectors stay valid
+
+  String::FlatContent sub_content = sub->GetFlatContent();
+  String::FlatContent pat_content = pat->GetFlatContent();
+
+  if (pat_content.IsOneByte()) {
+    Vector<const uint8_t> pat_vector = pat_content.ToOneByteVector();
+    if (sub_content.IsOneByte()) {
+      last_index = StringMatchBackwards(sub_content.ToOneByteVector(),
+                                        pat_vector, start_index);
+    } else {
+      last_index = StringMatchBackwards(sub_content.ToUC16Vector(), pat_vector,
+                                        start_index);
+    }
+  } else {
+    Vector<const uc16> pat_vector = pat_content.ToUC16Vector();
+    if (sub_content.IsOneByte()) {
+      last_index = StringMatchBackwards(sub_content.ToOneByteVector(),
+                                        pat_vector, start_index);
+    } else {
+      last_index = StringMatchBackwards(sub_content.ToUC16Vector(), pat_vector,
+                                        start_index);
+    }
+  }
+  return Smi::FromInt(last_index);
+}
+
 // ES6 section 21.1.3.10 String.prototype.localeCompare ( that )
 //
 // This function is implementation specific.  For now, we do not