Add ReplaceLineEndings and EnumerateLines #53115
Conversation
|
Tagging subscribers to this area: @tannergooding Issue DetailsResolves #40315. Note to reviewers - the API surface, devdoc, and general behavior are complete. However, the implementation is naïve, as it's just a simple iterator instead of SIMD-optimized. The reason for this is that there's currently no overload of IndexOfAny that fulfills the algorithmic complexity requirements we have. Once #49397 comes online it should be fairly straightforward to SIMD-optimize this without introducing layers of ISA special-casing or littering the workhorse method with unsafe code. But for now, let's get the basic functionality out and worry about optimizing it later.
|
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
I need to send a follow-up iteration which removes some of the duplicated code and calls these new APIs instead. |
src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/NewLineUtility.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/NewLineUtility.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/SpanLineEnumerator.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Text/NewLineUtility.cs
Outdated
Show resolved
Hide resolved
GrabYourPitchforks
requested review from
eiriktsarpalis,
jozkee,
layomia and
steveharter
as code owners
Changes in latest iteration
This last item in particular simplifies some of the unit test logic (particularly noticeable for System.Data). I only updated unit tests that appeared that they were not testing newline behavior as part of a protocol. For example, RFC 2616 (HTTP/1.1) gives a specific definition of newline chars which is incompatible with the Unicode's definition of newline chars, so I didn't update the System.Net.Http unit tests to use the new helper. Area owners, please verify that the changes I made to your unit tests are valid.
|
| s = s.Replace("\r", ""); | ||
| return s; | ||
| } | ||
| private static string NormalizeNewLines(string s) => s.ReplaceLineEndings(""); |
There was a problem hiding this comment.
Would s.ReplaceLineEndings() work the same?
There was a problem hiding this comment.
nvm, I just read the docs says it will replace with Environment.NewLine but I personally don't find it particularly obvious (I was expecting to remove them)
from the json perspective
|
Thanks all for the input! I'm going ahead and committing this for now, assuming everything is ok. System.Data crew (@ajcvickers @cheenamalhotra @David-Engel), if this feels inappropriate then please ping me, or open a new tracking issue, or simply revert the relevant portions of this commit. Whatever is easiest for you all. :) |
@GrabYourPitchforks Looks good to me. Plus the new code is so much cleaner! :) |
ghost
locked as resolved and limited conversation to collaborators
Resolves #40315.
Note to reviewers - the API surface, devdoc, and general behavior are complete. However, the implementation is naïve, as it's just a simple iterator instead of SIMD-optimized. The reason for this is that there's currently no overload of IndexOfAny that fulfills the algorithmic complexity requirements we have.
Once #49397 comes online it should be fairly straightforward to SIMD-optimize this without introducing layers of ISA special-casing or littering the workhorse method with unsafe code. But for now, let's get the basic functionality out and worry about optimizing it later.