Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Pass StringValues via in #2295

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Pass StringValues via in #2295

merged 1 commit into from
Mar 1, 2018

Conversation

benaadams
Copy link
Contributor

@benaadams benaadams commented Feb 4, 2018

Depends on dotnet/extensions#311 (for perf)

Resolves #2301

@halter73
Copy link
Member

This is already compiling despite dotnet/extensions#311 not being merged. Does the struct not need to be marked as readonly to use the in keyword for parameters? Do we still need to merge both PRs to avoid the copy?

@benaadams
Copy link
Contributor Author

Does the struct not need to be marked as readonly to use the in keyword for parameters?

Nope :-|

You can read fields of a non-readonly struct without it causing a copy; but not properties or methods (will cause copy)

Also changing to in is a code compatible change (not binary compatible).

Do we still need to merge both PRs to avoid the copy?

Yep

@benaadams
Copy link
Contributor Author

benaadams commented Feb 14, 2018

As aside it will give an error when in is used with a generic constraint dotnet/roslyn#24601 (comment) because its what can be specified on an interface; which is only properties and methods and no fields; thus is generally bad.

Is talk in that issue of adding a readonly struct constraint to allow it when it wouldn't be counter productive.

@halter73
Copy link
Member

I'm OK to merge this as soon as dotnet/extensions#311 is merged. I'll want to see benchmark numbers ofc.

@davidfowl
Copy link
Member

I'll merge it once the CI runs against the latest Common

@davidfowl davidfowl closed this Feb 28, 2018
@davidfowl davidfowl reopened this Feb 28, 2018
@davidfowl davidfowl merged commit 3004533 into aspnet:dev Mar 1, 2018
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.

4 participants