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

FrameworkName: Cache the string.Split char[] separator#11778

Merged
stephentoub merged 2 commits into
dotnet:masterfrom
justinvp:frameworkname_split
Sep 16, 2016
Merged

FrameworkName: Cache the string.Split char[] separator#11778
stephentoub merged 2 commits into
dotnet:masterfrom
justinvp:frameworkname_split

Conversation

@justinvp
Copy link
Copy Markdown
Contributor

Two commits for easier review, preferably merged as separate commits:

  1. Cleaned up the formatting to match the CoreFX coding style while making changes in this file.
  2. Modified the call to string.Split to use a cached separator array.

(I checked the test coverage and it's 100% for both line and branch coverage).

cc: @stephentoub, @weshaggard, @joperezr

Copy link
Copy Markdown
Member

@weshaggard weshaggard left a comment

Choose a reason for hiding this comment

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

LGTM

@justinvp
Copy link
Copy Markdown
Contributor Author

@weshaggard, if CoreFX can call the new Split methods in S.P.CoreLib, then is this PR (other than the formatting cleanup) unnecessary?

@weshaggard
Copy link
Copy Markdown
Member

@justinvp not all the builds in corefx can call the new API. Only the builds that are targeting .NET Core. For things targeting .NET Native or .NET Framework they still cannot call that new API yet. So at best we would have to #ifdef this code and I don't think that is worth it at this point.

@justinvp
Copy link
Copy Markdown
Contributor Author

@weshaggard, ah, ok. Thanks for the explanation.

@justinvp
Copy link
Copy Markdown
Contributor Author

@dotnet-bot Test Innerloop Ubuntu14.04 Release Build and Test please ("The build timed out, or was forcibly aborted after reaching the maximum length")

@stephentoub
Copy link
Copy Markdown
Member

@mmitche, http://dotnet-ci.cloudapp.net/job/dotnet_corefx/job/master/job/ubuntu14.04_release_prtest/229/ is another case where the log shows that there were no hung test suites, but the build was aborted after 2 hours.

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.

Thanks, Justin.

@stephentoub stephentoub merged commit a5e67d6 into dotnet:master Sep 16, 2016
@mmitche
Copy link
Copy Markdown
Member

mmitche commented Sep 16, 2016

I think these are msbuild failures, perhaps similar to what we saw before?

Msbuild was running. This is what it printed as it was forced to shutdown. How can we investigate this?

/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/Tools/Microsoft.Common.CurrentVersion.targets(3812,5): error MSB3030: Could not copy the file "/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/bin/obj/Unix.AnyCPU.Release/System.Net.NameResolution.Unit.Tests/System.Net.NameResolution.Unit.Tests.dll" because it was not found. [/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/src/System.Net.NameResolution/tests/UnitTests/System.Net.NameResolution.Unit.Tests.csproj]

@justinvp justinvp deleted the frameworkname_split branch September 16, 2016 20:27
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
FrameworkName: Cache the string.Split char[] separator

Commit migrated from dotnet/corefx@a5e67d6
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