From d8f06854543eab8f7f83906270308a995307f290 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 14 Feb 2020 15:28:01 -0800 Subject: [PATCH 1/2] Remove some calls to wstrcpy --- .../src/System/String.Manipulation.cs | 20 +++---- .../src/System/String.cs | 58 ++++++++++++------- 2 files changed, 47 insertions(+), 31 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 486145d6fb0b2a..4aaeeb0d643463 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -15,7 +15,7 @@ public partial class String { private const int StackallocIntBufferSizeLimit = 128; - private static unsafe void FillStringChecked(string dest, int destPos, string src) + private static void FillStringChecked(string dest, int destPos, string src) { Debug.Assert(dest != null); Debug.Assert(src != null); @@ -24,11 +24,10 @@ private static unsafe void FillStringChecked(string dest, int destPos, string sr throw new IndexOutOfRangeException(); } - fixed (char* pDest = &dest._firstChar) - fixed (char* pSrc = &src._firstChar) - { - wstrcpy(pDest + destPos, pSrc, src.Length); - } + Buffer.Memmove( + destination: ref Unsafe.Add(ref dest._firstChar, destPos), + source: ref src._firstChar, + elementCount: (uint)src.Length); } public static string Concat(object? arg0) => arg0?.ToString() ?? string.Empty; @@ -1672,11 +1671,10 @@ private unsafe string InternalSubString(int startIndex, int length) string result = FastAllocateString(length); - fixed (char* dest = &result._firstChar) - fixed (char* src = &_firstChar) - { - wstrcpy(dest, src + startIndex, length); - } + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref Unsafe.Add(ref _firstChar, startIndex)); return result; } diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index 6afb1c1da21eb0..159fd86b4cc1ba 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -58,11 +58,12 @@ private string Ctor(char[]? value) return Empty; string result = FastAllocateString(value.Length); - unsafe - { - fixed (char* dest = &result._firstChar, source = value) - wstrcpy(dest, source, value.Length); - } + + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref MemoryMarshal.GetArrayDataReference(value)); + return result; } @@ -91,11 +92,12 @@ private string Ctor(char[] value, int startIndex, int length) return Empty; string result = FastAllocateString(length); - unsafe - { - fixed (char* dest = &result._firstChar, source = value) - wstrcpy(dest, source + startIndex, length); - } + + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(value), startIndex)); + return result; } @@ -117,8 +119,12 @@ private unsafe string Ctor(char* ptr) return Empty; string result = FastAllocateString(count); - fixed (char* dest = &result._firstChar) - wstrcpy(dest, ptr, count); + + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref *ptr); + return result; } @@ -151,8 +157,12 @@ private unsafe string Ctor(char* ptr, int startIndex, int length) throw new ArgumentOutOfRangeException(nameof(ptr), SR.ArgumentOutOfRange_PartialWCHAR); string result = FastAllocateString(length); - fixed (char* dest = &result._firstChar) - wstrcpy(dest, pStart, length); + + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref *pStart); + return result; } @@ -398,20 +408,24 @@ public unsafe void CopyTo(int sourceIndex, char[] destination, int destinationIn } // Returns the entire string as an array of characters. - public unsafe char[] ToCharArray() + public char[] ToCharArray() { if (Length == 0) return Array.Empty(); char[] chars = new char[Length]; - fixed (char* src = &_firstChar, dest = &chars[0]) - wstrcpy(dest, src, Length); + + Buffer.Memmove( + destination: ref MemoryMarshal.GetArrayDataReference(chars), + source: ref _firstChar, + elementCount: (uint)Length); + return chars; } // Returns a substring of this string as an array of characters. // - public unsafe char[] ToCharArray(int startIndex, int length) + public char[] ToCharArray(int startIndex, int length) { // Range check everything. if (startIndex < 0 || startIndex > Length || startIndex > Length - length) @@ -425,8 +439,12 @@ public unsafe char[] ToCharArray(int startIndex, int length) } char[] chars = new char[length]; - fixed (char* src = &_firstChar, dest = &chars[0]) - wstrcpy(dest, src + startIndex, length); + + Buffer.Memmove( + destination: ref MemoryMarshal.GetArrayDataReference(chars), + source: ref Unsafe.Add(ref _firstChar, startIndex), + elementCount: (uint)length); + return chars; } From 42a7d6ff8ff121e49b09c7f0bc7ef7d2e88ac6b0 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Thu, 20 Feb 2020 16:06:21 -0800 Subject: [PATCH 2/2] PR feedback --- .../src/System/String.Manipulation.cs | 2 +- .../System.Private.CoreLib/src/System/String.cs | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 4aaeeb0d643463..a15d9b99950248 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1664,7 +1664,7 @@ public string Substring(int startIndex, int length) return InternalSubString(startIndex, length); } - private unsafe string InternalSubString(int startIndex, int length) + private string InternalSubString(int startIndex, int length) { Debug.Assert(startIndex >= 0 && startIndex <= this.Length, "StartIndex is out of range!"); Debug.Assert(length >= 0 && startIndex <= this.Length - length, "length is out of range!"); diff --git a/src/libraries/System.Private.CoreLib/src/System/String.cs b/src/libraries/System.Private.CoreLib/src/System/String.cs index a7b79e3716bb2b..ec6fc18d8debf1 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.cs @@ -380,8 +380,12 @@ public static unsafe string Copy(string str) throw new ArgumentNullException(nameof(str)); string result = FastAllocateString(str.Length); - fixed (char* dest = &result._firstChar, src = &str._firstChar) - wstrcpy(dest, src, str.Length); + + Buffer.Memmove( + elementCount: (uint)result.Length, // derefing Length now allows JIT to prove 'result' not null below + destination: ref result._firstChar, + source: ref str._firstChar); + return result; } @@ -403,8 +407,10 @@ public unsafe void CopyTo(int sourceIndex, char[] destination, int destinationIn if (destinationIndex > destination.Length - count || destinationIndex < 0) throw new ArgumentOutOfRangeException(nameof(destinationIndex), SR.ArgumentOutOfRange_IndexCount); - fixed (char* src = &_firstChar, dest = destination) - wstrcpy(dest + destinationIndex, src + sourceIndex, count); + Buffer.Memmove( + destination: ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(destination), destinationIndex), + source: ref Unsafe.Add(ref _firstChar, sourceIndex), + elementCount: (uint)count); } // Returns the entire string as an array of characters.