Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add EnumerateRunes() ref APIs and unit tests#33504

Merged
GrabYourPitchforks merged 4 commits intodotnet:masterfrom
GrabYourPitchforks:rune_enumerators
Nov 19, 2018
Merged

Add EnumerateRunes() ref APIs and unit tests#33504
GrabYourPitchforks merged 4 commits intodotnet:masterfrom
GrabYourPitchforks:rune_enumerators

Conversation

@GrabYourPitchforks
Copy link
Member

Reference assembly APIs and unit tests for dotnet/coreclr#21007.
This PR will not pass CI until the coreclr PR is committed and the build system injects the new drop into corefx.

public System.String TrimStart() { throw null; }
public System.String TrimStart(char trimChar) { throw null; }
public System.String TrimStart(params char[] trimChars) { throw null; }
public partial struct RuneEnumerator : System.Collections.Generic.IEnumerable<System.Text.Rune>, System.Collections.Generic.IEnumerator<System.Text.Rune>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public partial struct RuneEnumerator [](start = 8, length = 36)

Talked offline with Levi if this can be moved outside String class. I am no ta fan of nested types but I will be ok if decided to keep it inside.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next logical place would be System.Text.StringRuneEnumerator. Since very few people will ever use this type directly (most should access it via a foreach, which hides the type name) I'm fine with moving it out in order to keep the string class cleaner.

}
Assert.Equal(expected, enumeratedValues.ToArray());


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra space

@GrabYourPitchforks
Copy link
Member Author

It turns out we may have to move the nested class out to a standalone non-nested class anyway. System.String has a [TypeForwardedFrom(...)] attribute on it, which seems also to be applied to nested classes. This is causing unit test failures in the reflection layer. Should make @tarekgh happy to see it moved out. :)

@GrabYourPitchforks GrabYourPitchforks merged commit 1f9c47b into dotnet:master Nov 19, 2018
@GrabYourPitchforks GrabYourPitchforks deleted the rune_enumerators branch November 19, 2018 23:20
Anipik pushed a commit to dotnet/corert that referenced this pull request Nov 30, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/coreclr that referenced this pull request Nov 30, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jlennox pushed a commit to jlennox/corefx that referenced this pull request Dec 16, 2018
@karelz karelz added this to the 3.0 milestone Dec 21, 2018
@qbit86
Copy link

qbit86 commented Dec 26, 2018

StringRuneEnumerator implements both IEnumerator<Rune> and IEnumerable<Rune> (with GetEnumerator() => this). Does not this mean that “foreaching” over it is not idempotent? Are there in BCL any other examples of public stateful enumerators that are “one-time self-enumerable”?

@stephentoub
Copy link
Member

It's a struct. That makes a copy.

@qbit86
Copy link

qbit86 commented Dec 26, 2018

@stephentoub
I like the idea and consider using this approach in my libraries. But what are the actual guidelines for implementing IEnumerable<T> by enumerator? FDGs say:

X DO NOT use IEnumerator<T>, IEnumerator, or any other type that implements either of these interfaces, except as the return type of a GetEnumerator method.
X DO NOT implement both IEnumerator<T> and IEnumerable<T> on the same type.

As far as I can see, here are only three public types in corefx that utilize GetEnumerator() => this trick: StringRuneEnumerator, SpanRuneEnumerator, and BlobBuilder.Blobs.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants