Removing the struct/unmanaged constraint from the Vector types #87283
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsThis resolves #85893
|
| public static Vector<T> LoadAlignedNonTemporal<T>(T* source) | ||
| where T : unmanaged => LoadAligned(source); | ||
| public static Vector<T> LoadAlignedNonTemporal<T>(T* source) => LoadAligned(source); | ||
| #pragma warning restore CS8500 // This takes the address of, gets the size of, or declares a pointer to a managed type ('T') |
There was a problem hiding this comment.
This is unrelated, yes? It's fine, just confirming.
There was a problem hiding this comment.
Oh, nevermind, it is related, it was relying on unmanaged being a subset of struct.
There was a problem hiding this comment.
Which part in particular?
The change here is part of the general removal of the struct constraint so the code can be generally used in less constrained contexts.
This was originally where T : unmanaged because at the time C# didn't allow T* for unconstrained T. Now C# allows it, but surfaces a warning that T could be "managed".
If we didn't relax this, then Load wouldn't be callable from an unconstrained context and if we didn't suppress the warning then compilation would fail. Callers will get their own version of the warning if they are likewise not constrained to where T : unmanaged.
| { | ||
| T sum = default; | ||
| T sum = default!; |
There was a problem hiding this comment.
This makes me realize this change could actually introduce new warnings in 3rd party code. That's fine, just something to keep in mind. I'd been thinking of the removal of the constraint as being completely non-breaking.
There was a problem hiding this comment.
I don't think that's the case....
This warning is because we made an explicit change to the constraint such that its now unconstrained and now could be string, therefore default would be invalid for the specified T.
Existing callers must be where T : struct, so the compiler would consider T to be always non-nullable and the result to likewise be always non-nullable.
There was a problem hiding this comment.
Existing callers must be where T : struct
Ah, of course. I was thinking someone could have defined a helper function operating on VectorXx<T>, and inside that used default(T) without !, but as you say, they would have had to constrain T. So they'd have to change their code in some way for this to start triggering.
|
System.Private.Corelib is actually a bit smaller with this change, which is nice. CC. @vargaz, @SamMonoRT -- Are there any checks we need to do for the Mono side? The particular consideration is around whether the lack of constraint will negatively impact Mono AOT due to the existence of USG. Since its now unconstrained it "could" be say |
|
cc @lambdageek |
|
I don't think this will affect mono. |
This resolves #85893
This does not actually utilize the relaxed constraint anywhere yet. Give the churn involved in removing the constraints, I opted to leave that to a future PR.