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

Expose String.Trim overloads that take a single char#15334

Merged
weshaggard merged 1 commit intodotnet:masterfrom
justinvp:string_trim_tests
Jan 25, 2017
Merged

Expose String.Trim overloads that take a single char#15334
weshaggard merged 1 commit intodotnet:masterfrom
justinvp:string_trim_tests

Conversation

@justinvp
Copy link
Copy Markdown
Contributor

Expose and add tests for dotnet/coreclr#9009.

The new tests should compile/run on older platforms that don't have the new overloads yet since the existing overloads are params. Though, I assume it won't pass CI until the changes to coreclr are available to corefx due to the ref update.

Fixes #14337

@AlexGhiondea
Copy link
Copy Markdown
Contributor

@justinvp it looks like there is an APICompat failure with this change. Do you know if the actual implementation (from CoreCLR) has been ingested into CoreFx? If not, that could explain the failures.

@justinvp
Copy link
Copy Markdown
Contributor Author

@AlexGhiondea, dotnet/coreclr#9009 hasn't been merged yet and the changes haven't made their way to corefx yet.

@karelz karelz added the blocked Issue/PR is blocked on something - see comments label Jan 21, 2017
@karelz
Copy link
Copy Markdown
Member

karelz commented Jan 21, 2017

Blocked on coreclr change.

public System.String ToUpper(System.Globalization.CultureInfo culture) { throw null; }
public string ToUpperInvariant() { throw null; }
public string Trim() { throw null; }
#if netcoreapp11
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we do'nt need this anymore. @weshaggard ? (I can't find that PR where you explained this alreadY)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought I saw that explanation in another PR as well, but there were other netcoreapp11 ifdefs in this file, so I just matched that here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correct we no longer need these #ifdef's. @joperezr was cleaning some of these up but might have missed this project. @justinvp can you please go ahead and remove these #ifdefs for this project?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, but I likely won't be able to get to it until sometime Wednesday.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks like @joperezr beat me to it with #15291

@AlexGhiondea
Copy link
Copy Markdown
Contributor

@dotnet-bot test this please.

@weshaggard weshaggard removed the blocked Issue/PR is blocked on something - see comments label Jan 25, 2017
@justinvp
Copy link
Copy Markdown
Contributor Author

@dotnet-bot test Innerloop Windows_NT Release Build and Test

@justinvp
Copy link
Copy Markdown
Contributor Author

This should be good to merge now.

@weshaggard weshaggard merged commit 9f00738 into dotnet:master Jan 25, 2017
@justinvp justinvp deleted the string_trim_tests branch January 25, 2017 19:10
@karelz karelz modified the milestone: 2.0.0 Feb 4, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Expose String.Trim overloads that take a single char

Commit migrated from dotnet/corefx@9f00738
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants