Merge pull request #2090 from natalie-lang/string-delete

Make String#delete spec-compliant
natalie-lang · Jun 11, 2024 · 4864137 · 4864137
2 parents fbb92c0 + adce0e2
commit 4864137
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 22 deletions.
diff --git a/spec/core/string/delete_spec.rb b/spec/core/string/delete_spec.rb
@@ -1,4 +1,5 @@
 # -*- encoding: utf-8 -*-
+# frozen_string_literal: false
 require_relative '../../spec_helper'
 require_relative 'fixtures/classes'
 
@@ -54,19 +55,15 @@
   end
 
   it "respects backslash for escaping a -" do
-    NATFIXME 'respects backslash for escaping a -', exception: ArgumentError do
-      'Non-Authoritative Information'.delete(' \-\'').should ==
-        'NonAuthoritativeInformation'
-    end
+    'Non-Authoritative Information'.delete(' \-\'').should ==
+      'NonAuthoritativeInformation'
   end
 
   it "raises if the given ranges are invalid" do
     not_supported_on :opal do
-      NATFIXME 'Encodings', exception: SpecFailedException do
-        xFF = [0xFF].pack('C')
-        range = "\x00 - #{xFF}".force_encoding('utf-8')
-        -> { "hello".delete(range).should == "" }.should raise_error(ArgumentError)
-      end
+      xFF = [0xFF].pack('C')
+      range = "\x00 - #{xFF}".force_encoding('utf-8')
+      -> { "hello".delete(range).should == "" }.should raise_error(ArgumentError)
     end
     -> { "hello".delete("h-e") }.should raise_error(ArgumentError)
     -> { "hello".delete("^h-e") }.should raise_error(ArgumentError)

diff --git a/src/string_object.cpp b/src/string_object.cpp
@@ -33,27 +33,33 @@ static auto character_class_handler(Env *env, Args args) {
         auto new_selectors = Hashmap<String>(HashType::TMString);
         StringView last_character = {};
         bool negated = false;
-        // Split into selectors
-        auto it = selectors->begin();
-        if (*it == "^") {
-            ++it;
 
-            if (it != selectors->end()) {
+        // Split into selectors
+        size_t index = 0;
+        auto result = selectors->next_char_result(&index);
+        if (result.second == "^") {
+            if (index != selectors->bytesize())
                 negated = true;
-            } else {
+            else
                 new_selectors.set("^");
-            }
+            result = selectors->next_char_result(&index);
         }
-        for (; it != selectors->end(); ++it) {
-            auto value = *it;
-            if (value == "-" && last_character != StringView()) {
-                ++it;
-                if (it == selectors->end()) {
+        while (!result.second.is_empty()) {
+            if (!result.first)
+                env->raise("ArgumentError", "invalid byte sequence in {}", selectors->encoding()->name()->as_string()->string());
+
+            auto value = result.second;
+            if (value == "-" && last_character != StringView() && last_character != "\\") {
+                result = selectors->next_char_result(&index);
+                if (result.second.is_empty()) {
                     new_selectors.set(value.to_string());
                     break;
                 }
 
-                auto next_value = *it;
+                auto next_value = result.second;
+                if (!result.first)
+                    env->raise("ArgumentError", "invalid range \"{}-{}\" in string transliteration", last_character.to_string(), next_value.to_string());
+
                 if (last_character.to_string().cmp(next_value.to_string()) == 1)
                     env->raise("ArgumentError", "invalid range \"{}-{}\" in string transliteration", last_character.to_string(), next_value.to_string());
 
@@ -68,6 +74,7 @@ static auto character_class_handler(Env *env, Args args) {
                 last_character = value;
                 new_selectors.set(value.to_string());
             }
+            result = selectors->next_char_result(&index);
         }
 
         // intersect with current selectors