Validate ref/src parameter names #43838
Conversation
|
Tagging subscribers to this area: @safern, @ViktorHofer |
ericstj
added
the
breaking-change
ghost
added
the
needs-breaking-change-doc-created
| public static System.Numerics.Vector<T> Multiply<T>(System.Numerics.Vector<T> left, T right) where T : struct { throw null; } | ||
| public static System.Numerics.Vector<T> Multiply<T>(T left, System.Numerics.Vector<T> right) where T : struct { throw null; } | ||
| public static System.Numerics.Vector<System.Single> Narrow(System.Numerics.Vector<System.Double> source1, System.Numerics.Vector<System.Double> source2) { throw null; } | ||
| public static System.Numerics.Vector<System.Single> Narrow(System.Numerics.Vector<System.Double> low, System.Numerics.Vector<System.Double> high) { throw null; } |
There was a problem hiding this comment.
I'm fine with this as long as we are fine with the breaking change.
That being said, lower and upper may be better names than low and high here and would match the names we are using in the newer hardware intrinsics.
There was a problem hiding this comment.
I don't think we should be revisiting names at this point. We should choose either src or ref as the winners. So far we've felt that choosing src as the winner is slightly less breaking since a compile time break is easier to deal with than runtime break.
safern
left a comment
safern
left a comment
There was a problem hiding this comment.
🎉 -- as long we're fine with the breaking changes.
src/libraries/System.Security.Permissions/src/System/Data/Common/DBDataPermission.cs
Outdated
Show resolved
Hide resolved
Mono used a different name which was more consistent with the rest of the API.
ericstj
force-pushed
the
apiCompatOptional
branch
from
53241af to
051406b
Compare
|
@terrajobst do you support this change? Note a few places where I made parameter name changes in the implementation, or in both implementation/ref to match mono (which chose more consistent parameter names). |
| public static System.Attribute[] GetCustomAttributes(System.Reflection.MemberInfo element, System.Type attributeType) { throw null; } | ||
| public static System.Attribute[] GetCustomAttributes(System.Reflection.MemberInfo element, System.Type attributeType, bool inherit) { throw null; } |
There was a problem hiding this comment.
These changed both in ref and src to match Mono.
terrajobst
left a comment
terrajobst
left a comment
There was a problem hiding this comment.
I'm onboard with these kind of changes as it's inline with the kind of breaks we want to be able to do in .NET Core. The proposed changes seems fine.
I only have one question: it seems we're sometimes choosing ref and sometimes choosing src as the winner. Not that there is a problem but how did we choose? What makes more sense?
I preferred src over ref since they tended to have better names and it's less breaking (ref changes only break recompile). There were exceptions. Mentioned #43838 (comment), #43838 (comment), #43838 (comment), and changes in crypto that made things consistent with other similar APIs in the same assembly. |
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like @stephentoub's changes in #43834 broke this. I'll fix those up as well. |
|
Awesome. Thanks! |
ghost
locked as resolved and limited conversation to collaborators
ericstj
removed
the
needs-breaking-change-doc-created
|
Doc : dotnet/docs#26519 |
8 participants
No description provided.