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

Update the Index and Range interfaces and tests#36927

Closed
tarekgh wants to merge 3 commits intodotnet:masterfrom
tarekgh:UpdateIndexAndRangeInterfaces
Closed

Update the Index and Range interfaces and tests#36927
tarekgh wants to merge 3 commits intodotnet:masterfrom
tarekgh:UpdateIndexAndRangeInterfaces

Conversation

@tarekgh
Copy link
Copy Markdown
Member

@tarekgh tarekgh commented Apr 16, 2019

No description provided.

@tarekgh tarekgh added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 16, 2019
@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 16, 2019

This depends on dotnet/coreclr#24036

@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 16, 2019

@GrabYourPitchforks
Copy link
Copy Markdown
Member

Utf8String changes LGTM. Thanks!

@terrajobst
Copy link
Copy Markdown

Why is this marked NO MERGE? PResumably because we want to wait for dotnet/coreclr#24036 but this is just excluding APIs, so why would it matter?

@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 16, 2019

Why is this marked NO MERGE? PResumably because we want to wait for dotnet/coreclr#24036 but this is just excluding APIs, so why would it matter?

I am expecting the api compat checks will fail before getting the updated bits from coreclr. I am not expecting a compilation/runtime failure though. let's wait for the CI to see if this is the case or we can just merge it if it comes green.

@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 16, 2019

@terrajobst I confirmed the api compat failed because we have added the Slice method to Utf8String. I can fix that in this PR but this will cause it to break again when getting the updated coreclr bits.

@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 16, 2019

linking to the compiler fix dotnet/roslyn#35004 which will make the following test to compile and work fine

Assert.Throws<ArgumentOutOfRangeException>(() => new Span<char>(s.ToCharArray())[range])

@terrajobst
Copy link
Copy Markdown

let's wait for the CI to see if this is the case or we can just merge it if it comes green.

Makes sense. I'll wait untilt he CoreFX change is in so that I can update .NET Standard accordingly.

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice. Thanks.

@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented Apr 17, 2019

Closing this one as the commits included here are picked up in the PR #36976

@tarekgh tarekgh closed this Apr 17, 2019
@tarekgh tarekgh deleted the UpdateIndexAndRangeInterfaces branch April 17, 2019 21:18
@karelz karelz added this to the 3.0 milestone May 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

* NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants