Merge pull request #4734 from shuujii/simplify-arguments-check-in-String-rindex

Simplify arguments check in `String#rindex`

src/string.c

@@ -1977,21 +1977,17 @@ mrb_str_reverse(mrb_state *mrb, mrb_value str)
 /* 15.2.10.5.31 */
 /*
  *  call-seq:
- *     str.rindex(substring [, fixnum])   => fixnum or nil
- *     str.rindex(fixnum [, fixnum])   => fixnum or nil
- *     str.rindex(regexp [, fixnum])   => fixnum or nil
+ *     str.rindex(substring [, offset])   => fixnum or nil
  *
- *  Returns the index of the last occurrence of the given <i>substring</i>,
- *  character (<i>fixnum</i>), or pattern (<i>regexp</i>) in <i>str</i>. Returns
- *  <code>nil</code> if not found. If the second parameter is present, it
- *  specifies the position in the string to end the search---characters beyond
- *  this point will not be considered.
+ *  Returns the index of the last occurrence of the given <i>substring</i>.
+ *  Returns <code>nil</code> if not found. If the second parameter is
+ *  present, it specifies the position in the string to end the
+ *  search---characters beyond this point will not be considered.
  *
  *     "hello".rindex('e')             #=> 1
  *     "hello".rindex('l')             #=> 3
  *     "hello".rindex('a')             #=> nil
- *     "hello".rindex(101)             #=> 1
- *     "hello".rindex(/[aeiou]/, -2)   #=> 1
+ *     "hello".rindex('l', 2)          #=> 2
  */
 static mrb_value
 mrb_str_rindex(mrb_state *mrb, mrb_value str)
@@ -1999,46 +1995,25 @@ mrb_str_rindex(mrb_state *mrb, mrb_value str)
   mrb_value sub;
   mrb_int pos, len = RSTRING_CHAR_LEN(str);
 
-  switch (mrb_get_args(mrb, "|oi", &sub, &pos)) {
-    case 0:
-      sub = mrb_nil_value();
-      /* fall through */
-    case 1:
-      pos = len;
-      break;
-    case 2:
+  if (mrb_get_args(mrb, "S|i", &sub, &pos) == 1) {
+    pos = len;
+  }
+  else {
+    if (pos < 0) {
+      pos += len;
       if (pos < 0) {
-        pos += len;
-        if (pos < 0) {
-          return mrb_nil_value();
-        }
+        return mrb_nil_value();
       }
-      if (pos > len) pos = len;
-      break;
+    }
+    if (pos > len) pos = len;
   }
   pos = chars2bytes(str, 0, pos);
-
-  switch (mrb_type(sub)) {
-    default: {
-      mrb_value tmp;
-
-      tmp = mrb_check_string_type(mrb, sub);
-      if (mrb_nil_p(tmp)) {
-        mrb_raisef(mrb, E_TYPE_ERROR, "type mismatch: %v given", sub);
-      }
-      sub = tmp;
-    }
-     /* fall through */
-    case MRB_TT_STRING:
-      pos = str_rindex(mrb, str, sub, pos);
-      if (pos >= 0) {
-        pos = bytes2chars(RSTRING_PTR(str), RSTRING_LEN(str), pos);
-        BYTES_ALIGN_CHECK(pos);
-        return mrb_fixnum_value(pos);
-      }
-      break;
-
-  } /* end of switch (TYPE(sub)) */
+  pos = str_rindex(mrb, str, sub, pos);
+  if (pos >= 0) {
+    pos = bytes2chars(RSTRING_PTR(str), RSTRING_LEN(str), pos);
+    BYTES_ALIGN_CHECK(pos);
+    return mrb_fixnum_value(pos);
+  }
   return mrb_nil_value();
 }
 

test/t/string.rb

@@ -554,9 +554,16 @@ end if UTF8STRING
 
 assert('String#rindex', '15.2.10.5.31') do
   assert_equal 0, 'abc'.rindex('a')
+  assert_equal 0, 'abc'.rindex('a', 3)
+  assert_nil 'abc'.rindex('a', -4)
   assert_nil 'abc'.rindex('d')
+  assert_equal 6, 'abcabc'.rindex('')
+  assert_equal 3, 'abcabc'.rindex('a')
   assert_equal 0, 'abcabc'.rindex('a', 1)
   assert_equal 3, 'abcabc'.rindex('a', 4)
+  assert_equal 0, 'abcabc'.rindex('a', -4)
+  assert_raise(ArgumentError) { "hello".rindex }
+  assert_raise(TypeError) { "hello".rindex(101) }
 end
 
 assert('String#rindex(UTF-8)', '15.2.10.5.31') do
@@ -564,6 +571,7 @@ assert('String#rindex(UTF-8)', '15.2.10.5.31') do
   assert_nil str.rindex('さ')
   assert_equal 12, str.rindex('ち')
   assert_equal 3, str.rindex('ち', 10)
+  assert_equal 3, str.rindex('ち', -6)
 
   broken = "\xf0☀\xf1☁\xf2☂\xf3☃\xf0☀\xf1☁\xf2☂\xf3☃"
   assert_nil broken.rindex("\x81") # "\x81" is a part of "☁" ("\xe2\x98\x81")