Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Span<T> on all platforms that support it #6130

Merged
merged 1 commit into from Feb 9, 2021

Conversation

@ladipro
Copy link
Contributor

@ladipro ladipro commented Feb 8, 2021

Context

FEATURE_SPAN is defined only when building the .NET Core flavor of MSBuild but the feature has been supported everywhere but downlevel .NET Framework for some time.

Changes Made

Removed FEATURE_SPAN together with all code conditionally compiled when the flag was not defined. Wrapped the code in #if !CLR2COMPATIBILITY instead as it's not used when building MSBuildTaskHost.

Testing

Existing unit tests. There might be some very minor perf win on non-Windows OSes where we were previously using strings (i.e. Mono).

Notes

This is code cleanup.

@Forgind
Forgind approved these changes Feb 8, 2021
Copy link
Member

@Forgind Forgind left a comment

The only one I'd be at all worried about is net4, but that's probably ungrounded.

Copy link
Member

@BenVillalobos BenVillalobos left a comment

LGTM

@Forgind Forgind merged commit 31a98dc into dotnet:master Feb 9, 2021
7 checks passed
7 checks passed
license/cla All CLA requirements met.
Details
@azure-pipelines
msbuild-pr Build #20210208.2 succeeded
Details
@azure-pipelines
msbuild-pr (Linux Core) Linux Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Core) Windows Core succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full Release (no bootstrap)) Windows Full Release (no bootstrap) succeeded
Details
@azure-pipelines
msbuild-pr (Windows Full) Windows Full succeeded
Details
@azure-pipelines
msbuild-pr (macOS Core) macOS Core succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants