System.Memory source cleanup and fix byteOffset check in tests #26598

ahsonkhan · 2018-01-26T05:31:40Z

~~Part of the APIs proposed in https://github.com/dotnet/corefx/issues/~~

~~Remove~~
~~PadLeft, PadRight, and overloads~~
Removing unused using directives, leftover from Remove use of Unsafe in UTF-8 formatters, plus performance improvements #26289 (comment)
Fixing the byteOffset checks in Pin - related to Add MemoryPool apis #26563 (comment) / Fix Pin byteOffset bounds checks corefxlab#2071

Question:
If, on calling Remove, the entire source gets removed, i.e. there is nothing to copy into the destination, should we clear out the destination? Currently, I only clear as much as the source.Length (or the entire destination if source.Length is larger).

cc @jkotas, @stephentoub, @atsushikan, @dotnet/corefxlab-contrib, @tarekgh

KrzysztofCwalina · 2018-01-26T16:23:48Z

src/System.Memory/ref/System.Memory.cs

@@ -60,6 +60,11 @@ public static partial class MemoryExtensions
        public static bool Overlaps<T>(this System.ReadOnlySpan<T> first, System.ReadOnlySpan<T> second, out int elementOffset) { throw null; }
        public static bool Overlaps<T>(this System.Span<T> first, System.ReadOnlySpan<T> second) { throw null; }
        public static bool Overlaps<T>(this System.Span<T> first, System.ReadOnlySpan<T> second, out int elementOffset) { throw null; }
+        public static void PadLeft(this System.ReadOnlySpan<char> source, int totalWidth, System.Span<char> destination) { }
+        public static void PadLeft(this System.ReadOnlySpan<char> source, int totalWidth, System.Span<char> destination, char paddingChar) { }


Now that I look at the signature, I don't think we should have totalWidth in Span APIs. The width of the resulting string should be the size of the destination span. @terrajobst, @weshaggard?

We should answer this question by trying this API out on several real-world examples that use string.PadLeft/Right today.

Some examples we can look at:
https://github.com/PowerShell/PowerShell/blob/4bc52d2358222084738157a08907fac32d13bd3a/src/Microsoft.PowerShell.Commands.Utility/commands/utility/UtilityCommon.cs#L168

https://github.com/ShareX/ShareX/search?utf8=%E2%9C%93&q=PadLeft&type=

This one is interesting:
https://github.com/JeffreySu/WeiXinMPSDK/blob/5d62c88344f793d9da471670e31d989f5a3a542a/src/Senparc.Weixin/Senparc.Weixin/Helpers/EncryptHelper.cs#L268

Byte[] bKey = new Byte[32]; Array.Copy(Encoding.UTF8.GetBytes(Key.PadRight(bKey.Length)), bKey, bKey.Length);

Can be written as:

Byte[] bKey = new Byte[32]; Span<char> result = new char[bKey.Length]; ...(...(Key.AsReadOnlySpan().PadRight(result))...);

Atm, it would be:

Byte[] bKey = new Byte[32]; Span<char> result = new char[bKey.Length]; ...(...(Key.AsReadOnlySpan().PadRight(result.Length, result))...);

UtilityCommon.cs#L168

This looks like overengineered code. It can be written in much simpler way using X8 formatting string.

Byte[] bKey = new Byte[32];
Span result = new char[bKey.Length];
...(...(Key.AsReadOnlySpan().PadRight(result))...);

This has same number, or one more extra, allocation compared to original code. Switching to Span should naturally minimize or eliminate allocations. Otherwise, it is just an unnecessary churn.

This has same number, or one more extra, allocation compared to original code.

Fixed.

Byte[] bKey = new Byte[32]; Span<byte> bKeySpan = bKey; Span<char> bKeyChar = bKeySpan.NonPortableCast<byte, char>(); Key.AsReadOnlySpan().PadRight(bKeyChar.Length, bKeyChar); Encodings.Utf16.ToUtf8Length(bKeySpan, out int bytesNeeded); if (bytesNeeded > bKey.Length) throw new Exception(); OperationStatus result = Encodings.Utf16.ToUtf8(bKeySpan, bKeySpan, out int consumed, out int written); if (result != OperationStatus.Done) throw new Exception();

Switching to Span should naturally minimize or eliminate allocations.

If we include our encoding APIs, the span version definitely allocates less.

So you have replaced one easy to understand line with 8 lines of hard to understand code, that also uses non-portable APIs as bonus. This does not look like an improvement to me.

All this is in a crypto-stream initialization that allocates a ton of other stuff, so a few extra bytes saved is not going to make a difference.

That's fair. My intent was try to answer this question:

I don't think we should have totalWidth in Span APIs. The width of the resulting string should be the size of the destination span.

From the uses of PadLeft/PadRight, the totalWidth argument seems redundant. I rarely see the totalWidth < source.Length case.

KrzysztofCwalina · 2018-01-26T16:27:40Z

src/System.Memory/src/System/MemoryExtensions.cs

+                    return;
+                }
+
+                // destination is after source, reverse the order of copying


Why do you have to do it? Isn't our CopyTo smart enough to handle overlapping ranges? If it's not, should we make it so?

Our CopyTo is smart enough to handle overlapping ranges, but it does not help you here. This code has two CopyTo operations and they have to be ordered correctly.

Precisely. Since we are doing multiple copies, we have to do that check ourselves. If this API was in-place remove, we could get away without having this check.

Edit: Nevermind, these can't be in-place since we take in a ReadOnlySpan!

The can be in place; it's just in common scenarios they won't be as the source will come from a string. But I can just do:

Span<char> s = new char[100]; Remove(s.Slice(0, 50), 40, 10, s.Slice(30));

KrzysztofCwalina · 2018-01-26T16:29:15Z

src/System.Memory/src/System/MemoryExtensions.cs

+        /// <exception cref="System.ArgumentOutOfRangeException">
+        /// Thrown when either <paramref name="startIndex"/> or <paramref name="count"/> is less than zero or
+        /// when <paramref name="startIndex"/> plus <paramref name="count"/> specify a position outside the source span.
+        /// </exception>


Could you add remark to the dcos saying that it's ok for destination and span to be overlapping (including pointing to the same memory)

KrzysztofCwalina · 2018-01-26T16:31:18Z

src/System.Memory/src/System/MemoryExtensions.cs

+        /// Thrown when either <paramref name="startIndex"/> or <paramref name="count"/> is less than zero or
+        /// when <paramref name="startIndex"/> plus <paramref name="count"/> specify a position outside the source span.
+        /// </exception>
+        public static void Remove(this ReadOnlySpan<char> source, int startIndex, int count, Span<char> destination)


Would it make sense for the method to either return the length of the result or require the destination buffer to be exactly the size of the result. As it stands right now, the caller has to compute the size of the result.

@KrzysztofCwalina Have we looked at Span libraries in other programming environments and see what their design patterns are? Do they have Remove API like this?

This Remove API does not feel right. I do not see why anybody would ever want to use it instead of just a simple CopyTo that is much easier to understand.

I agree with Jan here. It can be confusing too to have it. This API can be a useful APII only if we make it removing some characters patterns from the source span. but with the proposed behavior, CopyTo should be enough and clearer.

I agree. I think it would be better for this API to be in-place instead of taking in a destination buffer, especially given we already deviate from the string.Remove signature here.

This API is for transitioning existing string code to the world of spans. I think it's not relevant if other languages have it or not. The motivation is that string has these APIs.

But, I agree with removing this particular method. I think it does too much dancing around the overlapping spans. In real scenarios, the caller will know the relative locations of the buffers and will be able to write much simpler code.

This API is for transitioning existing string code to the world of spans

Then we should do a little validation for each of these APIs that they actually help with transitioning existing code to the world of spans. Pick a few real world examples and try to transition them over to Spans using these APIs.

For example, here is randomly choosen use of string.Remove in corefx: https://github.com/dotnet/corefx/blob/master/src/System.Management/src/System/Management/ManagementQuery.cs#L122 . What we want this to look like when transitioned to Spans?

This isn't necessarily a good example since all the calls to Remove here can be replaced with Slice (since characters are only being removed from the front). The real scenario would be something that requires removal from the middle.

In any case, it could end up looking like this, but it won't use Remove (not tested):

internal static void ParseToken(ref ReadOnlySpan<char> q, ReadOnlySpan<char> token, ReadOnlySpan<char> op, ref bool bTokenFound, ref ReadOnlySpan<char> tokenValue) { if (bTokenFound) throw new ArgumentException(); // Invalid query - duplicate token bTokenFound = true; q = q.Slice(token.Length).TrimStart(null); if (0 != q.IndexOf(op)) throw new ArgumentException(); // Invalid query // Strip off the op and any leading WS q = q.Slice(op.Length).TrimStart(null); if (0 == q.Length) throw new ArgumentException(); // Invalid query - token has no value // Next token should be the token value - look for terminating WS // or end of string int i; if (-1 == (i = q.IndexOf(' '))) i = q.Length; // No WS => consume entire string tokenValue = q.Slice(0, i); q = q.Slice(tokenValue.Length).TrimStart(null); }

If we insist on using Remove instead, maybe something like this:

internal static string ParseToken(ReadOnlySpan<char> q, ReadOnlySpan<char> token, ReadOnlySpan<char> op, ref bool bTokenFound, out int bytesConsumed) { if (bTokenFound) throw new ArgumentException(); // Invalid query - duplicate token bTokenFound = true; Span<char> result = new char[q.Length]; q.Remove(0, token.Length, result); result.AsReadOnlySpan().TrimStart(null); if (0 != result.IndexOf(op)) throw new ArgumentException(); // Invalid query // Strip off the op and any leading WS result.AsReadOnlySpan().Remove(0, op.Length, result); result.AsReadOnlySpan().TrimStart(null); if (0 == result.Length) throw new ArgumentException(); // Invalid query - token has no value // Next token should be the token value - look for terminating WS // or end of string int i; if (-1 == (i = result.IndexOf(' '))) i = result.Length; // No WS => consume entire string bytesConsumed = result.Slice(i).AsReadOnlySpan().TrimStart(null).Length; return new string(result.Slice(0, i).ToArray()); }

An example where startIndex != 0 is:
https://github.com/dotnet/corefx/blob/master/src/System.Private.Uri/src/System/Uri.cs#L957

But even then, we can work around it since startIndex is a known constant:

if (NotAny(Flags.HostNotCanonical | Flags.PathNotCanonical | Flags.ShouldBeCompressed)) { start = IsUncPath ? _info.Offset.Host - 2 : _info.Offset.Path; ReadOnlySpan<char> str = _string.AsReadOnlySpan(); if (!(IsImplicitFile && _info.Offset.Host == (IsDosPath ? 0 : 2) && info.Offset.Query == _info.Offset.End)) { str = (IsDosPath && (str[start] == '/' || str[start] == '\\')) ? str.Slice(start + 1, _info.Offset.Query - start - 1) : str.Slice(start, _info.Offset.Query - start); } // Should be a rare case, convert c|\ into c:\ if (IsDosPath && str[1] == '|') { str[1] = ':'; }

str[1] = ':';

str is read-only in this example. I think you would want something like this:

char[] arr = str.ToArray(); arr[1] = '1'; str = arr.AsReadOnlySpan();

}

It does one allocation compared to two in the current string-based code.

Yes. You are right.

I will get rid of Remove() as we have it currently then. It doesn't seem necessary and the current API signature just adds complexity.

KrzysztofCwalina · 2018-01-26T16:32:06Z

src/System.Memory/src/System/MemoryExtensions.cs

+                }
+            }
+            else
+            {   // Remove everything


Do we really want to pay the cost of clearing here. Why?

We are only clearing whatever falls within source.Length - count. Nothing beyond that is being cleared. See tests like https://github.com/dotnet/corefx/pull/26598/files#diff-e1048de19afdbf1765ab34e0dfea880cR255

KrzysztofCwalina · 2018-01-26T16:34:48Z

src/System.Memory/src/System/MemoryExtensions.cs

+            else
+            {
+                source.CopyTo(destination.Slice(count));
+                for (int i = 0; i < count; i++)


Would it be faster to use Fill?

It was slower to do so, especially for small amounts, similar to #26289 (comment).

We also avoid an unnecessary Slice.

What does that say about our Fill API if we can't use it? Do we successfully use it anywhere you've done perf measurements? Maybe Fill should just be fixed.

Maybe Fill should just be fixed.

https://github.com/dotnet/corefx/issues/26604

stephentoub · 2018-01-26T14:52:25Z

src/System.Memory/src/System/MemoryExtensions.cs

+        }
+
+        /// <summary>
+        /// Moves the source characters to the destination and right-aligns them by padding with


"Move" generally suggests removing from one and along to the other. "Copy" would be better. Same for other similar uses.

stephentoub · 2018-01-26T14:55:23Z

src/System.Memory/src/System/MemoryExtensions.cs

+            {
+                source.CopyTo(destination.Slice(count));
+                for (int i = 0; i < count; i++)
+                    destination[i] = ' ';


I will file an issue to improve Fill perf so it becomes useful.

stephentoub · 2018-01-26T15:03:02Z

src/System.Memory/src/System/MemoryExtensions.cs

+        /// Thrown when <paramref name="totalWidth"/> is less than zero.
+        /// </exception>
+        public static void PadLeft(this ReadOnlySpan<char> source, int totalWidth, Span<char> destination)
+        {


How do we decide when to have two overloads vs one with a default value? And why doesn't this overload just call the other specifying ' ' as the char?

And why doesn't this overload just call the other specifying ' ' as the char?

I will fix that.

How do we decide when to have two overloads vs one with a default value?

The motivation here was matching existing string APIs. I am fine with one method with the default value being ' '.

I am fine with one method with the default value being ' '.

I don't have a preference; it was actually a question. We seem to be a bit inconsistent with some of the new APIs we're adding.

I am not sure what the guidance is. To be consistent with string, I will keep them as two overloads, unless anyone feels strongly otherwise.

stephentoub · 2018-01-26T15:06:23Z

src/System.Memory/src/System/MemoryExtensions.cs

+                if (source.Length < destination.Length)
+                    destination.Slice(0, source.Length).Clear();
+                else
+                    destination.Clear();


Why do we clear? Presumably since we don't output number of chars written, we expect the caller will use source.Length - count as the number of valid chars in the dest; why would they assume anything beyond that had changed?

We are not clearing anything beyond source.Length - count. Here, destination.Length < source.Length. There are tests with canaries just beyond that, that remain in tact.

I don't understand. We're only here if source.Length-count is 0. So we shouldn't be clearing anything. What am I missing? There should be nothing to copy and we should be done.

Look at this test: https://github.com/dotnet/corefx/pull/26598/files#diff-e1048de19afdbf1765ab34e0dfea880cR162

source = "abc"; destination = "ab"; source.Remove(0, source.Length, destination); // Semantically, we want everything removed here // Should destination be "/0/0" now or should it remain "ab"?!

I agree that the behavior is more complex than it needs to be, essentially due to the fact that we take in two buffers. I think this (along with other) issues get resolved with an in-place implementation.

Edit: Can't do in-place. string -> ReadOnlySpan, i.e. not writable. Ty @KrzysztofCwalina for clarifying.

If the API does anything more than effectively copy the remaining 1 or 2 pieces, it makes little sense to me. If everything is removed, the destination should remain untouched; I don't know how to reason about anything else.

And if there are no remaining pieces? The destination should remain untouched as well? I am fine with that being the semantics of this method.

And if there are no remaining pieces? The destination should remain untouched as well?

Right, then there's nothing to be done, nothing to copy, nothing to write.

All that said, I have the same questions about usefulness that Jan raised. For reference, here's the two places I needed a Remove substitute:
https://github.com/dotnet/coreclr/blob/d08b5fd08ea73bebe4acd1af842ef8f212429944/src/mscorlib/shared/System/Globalization/DateTimeParse.cs#L5296
https://github.com/dotnet/coreclr/blob/d08b5fd08ea73bebe4acd1af842ef8f212429944/src/mscorlib/shared/System/Globalization/DateTimeParse.cs#L5320
and what I used:
https://github.com/dotnet/coreclr/blob/d08b5fd08ea73bebe4acd1af842ef8f212429944/src/mscorlib/shared/System/StringSpanHelpers.cs#L106-L128

I had looked at this design and it works just fine because it allocates. Maybe that is the only feasible alternative. A non-allocating Remove() has the quirks that are being discussed here, which potentially reduces its usefulness.

If we are fine with allocating, we could re-write RemoveTrailingInQuoteSpaces without using Remove as follows (essentially inlining the call). And this ends up being faster since the user has specific info which means certain operations can be skipped.

internal void RemoveTrailingInQuoteSpaces() { int i = Length - 1; if (i <= 1) { return; } char ch = Value[i]; // Check if the last character is a quote. if (ch == '\'' || ch == '\"') { if (Char.IsWhiteSpace(Value[i - 1])) { i--; while (i >= 1 && Char.IsWhiteSpace(Value[i - 1])) { i--; } // Value = Value.Remove(i, Value.Length - 1 - i); Span<char> result = new char[i + 1]; result[i] = ch; Value.Slice(0, i).CopyTo(result); Value = result.AsReadOnlySpan(); } } } internal void RemoveLeadingInQuoteSpaces() { if (Length <= 2) { return; } int i = 0; char ch = Value[i]; // Check if the last character is a quote. if (ch == '\'' || ch == '\"') { while ((i + 1) < Length && Char.IsWhiteSpace(Value[i + 1])) { i++; } if (i != 0) { //Value = Value.Remove(1, i); Span<char> result = new char[Value.Length - i]; result[0] = ch; Value.Slice(i + 1).CopyTo(result.Slice(1)); Value = result.AsReadOnlySpan(); } } }

KrzysztofCwalina · 2018-02-01T00:34:09Z

src/System.Memory/src/System/MemoryExtensions.cs

-        /// Thrown when <paramref name="totalWidth"/> is less than zero.
-        /// </exception>
-        public static void PadLeft(this ReadOnlySpan<char> source, int totalWidth, Span<char> destination, char paddingChar)
+        public static void PadLeft(this ReadOnlySpan<char> source, Span<char> destination, char paddingChar)


Do we feel this (and similar) methods are useful? They are literally just copy and fill.
@stephentoub, I would be interested especially in your opinion as you requested these (string-like) APIs after trying to port code from string to 'Span', but I am not sure which of these new members would actually help in the port.

I would be interested especially in your opinion as you requested these (string-like) APIs after trying to port code from string to 'Span', but I am not sure which of these new members would actually help in the port.

To me, the most important string-like methods we can provide are those for comparison/equality. Doing it correctly in the face of cultures is challenging outside of the platform and requires a dependency on the OS or a library like ICU. Even just for ordinal it's difficult to do efficiently, with optional handling of casing. This is my Pri0, methods like Compare, Equals, StartsWith, etc.

My Pri1 is support for trimming and searching, methods like Trim and IndexOf. These are possible for a dev to write, handfuls of lines of code, but they're so commonly used, I believe it's important that we have built-in support for these that we can implement once and tune and make available for all.

You can see at https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/StringSpanHelpers.cs the APIs I needed in support of the span work in corelib itself; the methods I mentioned above account for almost all of the methods in that file. The only other one is Remove, which I concede is an oddity due to it needing to allocate in some situations. Then again, the implementation in that file is obviously missing some special cases where it wouldn't need to allocate (e.g. where it's just removing from the beginning or end), which would necessitate more code, which highlights that even something like that can be useful for us to provide.

As for PadLeft/PadRight, I can probably count on two hands the number of times I've used those even with strings, so they're not high on my priority list :)

This is my Pri0, methods like Compare, Equals, StartsWith, etc.

Working on these next :)

@terrajobst, @jkotas, @joshfree, @stephentoub, should we remove the Pad APIs too? What are your thoughts?

should we remove the Pad APIs too? What are your thoughts?

Let's remove them for now. We can add them in the future if we find a real need.

I agree with @stephentoub

OK. Updated the PR.

ahsonkhan · 2018-02-02T03:24:36Z

@dotnet-bot test NETFX x86 Release Build

ahsonkhan · 2018-02-02T03:25:59Z

https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netfx+CGroup_Release+AGroup_x86+TestOuter_false_prtest/7720/consoleFull

16:15:46       System.Net.Http.Functional.Tests.HttpRetryProtocolTests.GetAsync_RetryOnConnectionClosed_Success [FAIL]
16:15:46         System.Net.Http.HttpRequestException : An error occurred while sending the request.
16:15:46         ---- System.Net.WebException : The request was aborted: The request was canceled.
16:15:46         Stack Trace:
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46              at System.Net.Http.HttpClient.<FinishSendAsyncBuffered>d__58.MoveNext()
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46              at System.Net.Http.Functional.Tests.HttpRetryProtocolTests.<<GetAsync_RetryOnConnectionClosed_Success>b__5_0>d.MoveNext()
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46              at System.Net.Http.Functional.Tests.HttpRetryProtocolTests.<>c__DisplayClass4_0.<<CreateServerAndClientAsync>b__0>d.MoveNext()
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46              at System.Net.Test.Common.LoopbackServer.<>c__DisplayClass3_0.<CreateServerAsync>b__0(Task t)
16:15:46     Discovering: System.Net.WebHeaderCollection.Tests
16:15:46              at System.Threading.Tasks.Task.Execute()
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46              at System.Net.Http.Functional.Tests.HttpRetryProtocolTests.<GetAsync_RetryOnConnectionClosed_Success>d__5.MoveNext()
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46           --- End of stack trace from previous location where exception was thrown ---
16:15:46              at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
16:15:46              at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
16:15:46           ----- Inner Stack Trace -----
16:15:46              at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
16:15:46              at System.Net.Http.HttpClientHandler.GetResponseCallback(IAsyncResult ar)

stephentoub · 2018-02-02T14:43:53Z

@ahsonkhan, if you wouldn't mind, when you get failures like that, please add a note about it to an existing issue for the failing test if there is one, or otherwise open a new issue with the details. I noted this one here: https://github.com/dotnet/corefx/issues/26770.

stephentoub · 2018-02-02T14:48:25Z

@dotnet-bot test OSX x64 Debug Build please (#26790)

ahsonkhan · 2018-02-02T23:57:19Z

@dotnet-bot test OSX x64 Debug Build

…t/corefx#26598) * String-like Span extension methods - Remove / PadLeft and Right * Get rid off Remove, update PadLeft/Right and address feedback. * Update PadLeft and PadRight tests since we removed totalWidth * Getting rid of PadLeft and PadRight APIs. Add later if needed. Commit migrated from dotnet/corefx@1d19c3a

String-like Span extension methods - Remove / PadLeft and Right

2ce066c

ahsonkhan added the area-System.Memory label Jan 26, 2018

ahsonkhan self-assigned this Jan 26, 2018

KrzysztofCwalina reviewed Jan 26, 2018

View reviewed changes

stephentoub reviewed Jan 26, 2018

View reviewed changes

jkotas mentioned this pull request Jan 28, 2018

Span overload added for path apis dotnet/coreclr#15608

Merged

ahsonkhan added 2 commits January 31, 2018 15:01

Get rid off Remove, update PadLeft/Right and address feedback.

fd90303

Update PadLeft and PadRight tests since we removed totalWidth

4f481e4

KrzysztofCwalina reviewed Feb 1, 2018

View reviewed changes

Getting rid of PadLeft and PadRight APIs. Add later if needed.

dbc1fc2

ahsonkhan changed the title ~~String-like Span extension methods - Remove / PadLeft and Right~~ System.Memory source cleanup and fix byteOffset check in tests Feb 2, 2018

jkotas approved these changes Feb 2, 2018

View reviewed changes

stephentoub approved these changes Feb 2, 2018

View reviewed changes

ahsonkhan mentioned this pull request Feb 3, 2018

Remove use of Unsafe in UTF-8 formatters, plus performance improvements #26289

Merged

ahsonkhan merged commit 1d19c3a into dotnet:master Feb 3, 2018

ahsonkhan deleted the StringExtensions branch February 3, 2018 02:20

jkotas mentioned this pull request Feb 4, 2018

Add Span/ReadOnlySpan TryCast #26662

Closed

karelz added this to the 2.1.0 milestone Feb 4, 2018

ahsonkhan mentioned this pull request Jan 31, 2020

Investigate ways to optimize Span.Fill and Span.Clear for small buffers dotnet/runtime#24806

Closed

System.Memory source cleanup and fix byteOffset check in tests #26598

System.Memory source cleanup and fix byteOffset check in tests #26598

Conversation

ahsonkhan commented Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Jan 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

stephentoub Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

stephentoub Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan commented Feb 2, 2018

ahsonkhan commented Feb 2, 2018

stephentoub commented Feb 2, 2018

stephentoub commented Feb 2, 2018

ahsonkhan commented Feb 2, 2018

ahsonkhan commented Jan 26, 2018 •

edited

Loading

jkotas Jan 27, 2018 •

edited

Loading

ahsonkhan Jan 27, 2018 •

edited

Loading

ahsonkhan Jan 26, 2018 •

edited

Loading

jkotas Jan 26, 2018 •

edited

Loading

jkotas Jan 26, 2018 •

edited

Loading

ahsonkhan Jan 27, 2018 •

edited

Loading

ahsonkhan Jan 27, 2018 •

edited

Loading

jkotas Jan 27, 2018 •

edited

Loading

stephentoub Jan 26, 2018 •

edited

Loading

ahsonkhan Jan 26, 2018 •

edited

Loading

ahsonkhan Jan 26, 2018 •

edited

Loading

stephentoub Jan 26, 2018 •

edited

Loading

ahsonkhan Jan 26, 2018 •

edited

Loading

stephentoub Jan 26, 2018 •

edited

Loading

stephentoub Jan 26, 2018 •

edited

Loading

ahsonkhan Jan 26, 2018 •

edited

Loading