Merge pull request #238 from Joy-less/simplify-replace

Simplify & optimize Replace

Author: Joy-less

CHANGELOG.md

@@ -6,6 +6,15 @@ All notable changes to **ValueStringBuilder** will be documented in this file. T
 
 ## [Unreleased]
 
+### Changed
+
+- Optimized and simplified `Replace` (by @Joy-less in #238)
+- Simplified `IndexOf` and `LastIndexOf` (by @Joy-less in #238)
+
+### Fixed
+
+- Fixed `IndexOf` and `LastIndexOf` allowing out-of-bounds index when the string to find is empty (by @Joy-less in #238)
+
 ## [2.3.1] - 2025-02-20
 
 ### Changed

src/LinkDotNet.StringBuilder/IntegerSpanList.cs

@@ -92,53 +92,45 @@ public void Replace(scoped ReadOnlySpan<char> oldValue, scoped ReadOnlySpan<char
         ArgumentOutOfRangeException.ThrowIfLessThan(startIndex, 0);
         ArgumentOutOfRangeException.ThrowIfGreaterThan(startIndex + count, Length);
 
-        var length = startIndex + count;
-        var slice = buffer[startIndex..length];
-
-        if (oldValue.SequenceEqual(newValue))
+        if (oldValue.IsEmpty || oldValue.Equals(newValue, StringComparison.Ordinal))
         {
             return;
         }
 
-        // We might want to check whether or not we want to introduce different
-        // string search algorithms for longer strings.
-        // I had checked initially with Boyer-Moore but it didn't make that much sense as we
-        // don't expect very long strings and then the performance is literally the same. So I went with the easier solution.
-        var hits = NaiveSearch.FindAll(slice, oldValue);
+        var index = startIndex;
+        var remainingChars = count;
 
-        if (hits.IsEmpty)
+        while (remainingChars > 0)
         {
-            return;
-        }
-
-        var delta = newValue.Length - oldValue.Length;
+            var foundSubIndex = buffer.Slice(index, remainingChars).IndexOf(oldValue, StringComparison.Ordinal);
+            if (foundSubIndex < 0)
+            {
+                break;
+            }
 
-        for (var i = 0; i < hits.Length; i++)
-        {
-            var index = startIndex + hits[i] + (delta * i);
+            index += foundSubIndex;
+            remainingChars -= foundSubIndex;
 
-            // newValue is smaller than old value
-            // We can insert the slice and remove the overhead
-            if (delta < 0)
+            if (newValue.Length == oldValue.Length)
             {
+                // Just replace the old slice
                 newValue.CopyTo(buffer[index..]);
-                Remove(index + newValue.Length, -delta);
             }
-
-            // Same length -> We can just replace the memory slice
-            else if (delta == 0)
+            else if (newValue.Length < oldValue.Length)
             {
+                // Replace the old slice and trim the unused slice
                 newValue.CopyTo(buffer[index..]);
+                Remove(index + newValue.Length, oldValue.Length - newValue.Length);
             }
-
-            // newValue is larger than the old value
-            // First add until the old memory region
-            // and insert afterwards the rest
             else
             {
+                // Replace the old slice and append the extra slice
                 newValue[..oldValue.Length].CopyTo(buffer[index..]);
                 Insert(index + oldValue.Length, newValue[oldValue.Length..]);
             }
+
+            index += newValue.Length;
+            remainingChars -= oldValue.Length;
         }
     }
 
@@ -153,7 +145,7 @@ public void Replace(scoped ReadOnlySpan<char> oldValue, scoped ReadOnlySpan<char
     /// </remarks>
     /// /// <typeparam name="T">Any type.</typeparam>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public void ReplaceGeneric<T>(ReadOnlySpan<char> oldValue, T newValue)
+    public void ReplaceGeneric<T>(scoped ReadOnlySpan<char> oldValue, T newValue)
         => ReplaceGeneric(oldValue, newValue, 0, Length);
 
     /// <summary>
@@ -164,24 +156,23 @@ public void ReplaceGeneric<T>(ReadOnlySpan<char> oldValue, T newValue)
     /// <param name="startIndex">The index to start in this builder.</param>
     /// <param name="count">The number of characters to read in this builder.</param>
     /// <remarks>
-    /// If <paramref name="newValue"/> is from type <see cref="ISpanFormattable"/> an optimized version is taken.
-    /// Otherwise the ToString method is called.
+    /// If <paramref name="newValue"/> is <see cref="ISpanFormattable"/>, <c>TryFormat</c> is used.
+    /// Otherwise, <c>ToString</c> is used.
     /// </remarks>
     /// /// <typeparam name="T">Any type.</typeparam>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public void ReplaceGeneric<T>(ReadOnlySpan<char> oldValue, T newValue, int startIndex, int count)
+    public void ReplaceGeneric<T>(scoped ReadOnlySpan<char> oldValue, T newValue, int startIndex, int count)
     {
         if (newValue is ISpanFormattable spanFormattable)
         {
-            Span<char> tempBuffer = stackalloc char[24];
+            Span<char> tempBuffer = stackalloc char[128];
             if (spanFormattable.TryFormat(tempBuffer, out var written, default, null))
             {
                 Replace(oldValue, tempBuffer[..written], startIndex, count);
+                return;
             }
         }
-        else
-        {
-            Replace(oldValue, (newValue?.ToString() ?? string.Empty).AsSpan(), startIndex, count);
-        }
+
+        Replace(oldValue, newValue?.ToString() ?? string.Empty, startIndex, count);
     }
 }

src/LinkDotNet.StringBuilder/NaiveSearch.cs

@@ -209,12 +209,7 @@ public void Remove(int startIndex, int length)
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     public readonly int IndexOf(ReadOnlySpan<char> word, int startIndex)
     {
-        if (startIndex < 0)
-        {
-            throw new ArgumentOutOfRangeException(nameof(startIndex), "Start index can't be smaller than 0.");
-        }
-
-        return word.IsEmpty ? 0 : NaiveSearch.FindFirst(buffer[startIndex..bufferPosition], word);
+        return buffer[startIndex..bufferPosition].IndexOf(word);
     }
 
     /// <summary>
@@ -234,12 +229,7 @@ public readonly int IndexOf(ReadOnlySpan<char> word, int startIndex)
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
     public readonly int LastIndexOf(ReadOnlySpan<char> word, int startIndex)
     {
-        if (startIndex < 0)
-        {
-            throw new ArgumentOutOfRangeException(nameof(startIndex), "Start index can't be smaller than 0.");
-        }
-
-        return word.IsEmpty ? 0 : NaiveSearch.FindLast(buffer[startIndex..bufferPosition], word);
+        return buffer[startIndex..bufferPosition].LastIndexOf(word);
     }
 
     /// <summary>

src/LinkDotNet.StringBuilder/ValueStringBuilder.Replace.cs

@@ -295,7 +295,7 @@ public void ShouldReturnZeroWhenEmptyStringInIndexOf()
         using var builder = new ValueStringBuilder();
         builder.Append("Hello");
 
-        var index = builder.IndexOf(string.Empty, 6);
+        var index = builder.IndexOf(string.Empty, 5);
 
         index.ShouldBe(0);
     }
@@ -316,7 +316,7 @@ public void ShouldReturnZeroWhenEmptyStringInLastIndexOf()
         using var builder = new ValueStringBuilder();
         builder.Append("Hello");
 
-        var index = builder.LastIndexOf(string.Empty, 6);
+        var index = builder.LastIndexOf(string.Empty, 5);
 
         index.ShouldBe(0);
     }