Mark DivRem with RequiresPreviewFeatures #82221
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to 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-runtime-intrinsics Issue Details
|
| [System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")] | ||
| public static (nuint Quotient, nuint Remainder) DivRem(nuint lower, nuint upper, nuint divisor) { throw null; } | ||
| [System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")] | ||
| public static (nint Quotient, nint Remainder) DivRem(nuint lower, nint upper, nint divisor) { throw null; } |
There was a problem hiding this comment.
Have these APIs gone through API review?
There was a problem hiding this comment.
I assume that you are asking about this specific overload - I do not see it in the approved set. All overloads are in the approved set - I was looking in a wrong place.
There was a problem hiding this comment.
The general premise of extending the intrinsics to support nint/nuint was reviewed and approved already: #52021
AFAIR, we covered at the time in API review that any other already approved but NYI APIs were also covered by that.
There was a problem hiding this comment.
The same general approval for nint/nuint for Arm64 was done here: #52027
There was a problem hiding this comment.
Indeed, it's covered in the approval explicitly: #27292 (comment)
| @@ -5217,11 +5217,23 @@ public abstract partial class X86Base | |||
| internal X86Base() { } | |||
| public static bool IsSupported { get { throw null; } } | |||
| public static (int Eax, int Ebx, int Ecx, int Edx) CpuId(int functionId, int subFunctionId) { throw null; } | |||
| [System.Runtime.Versioning.RequiresPreviewFeaturesAttribute("DivRem is in preview.")] | |||
There was a problem hiding this comment.
Is the plan that we ship these in .NET 8 as preview, or is the plan that by the time we ship .NET 8 it's fully supported?
There was a problem hiding this comment.
Yes, we plan to address it in .NET 8. Tracking issue #82194
There was a problem hiding this comment.
Ok. Technically every new API we add in previews is preview and doesn't require such attribution; this attribute was really intended to be used for things to remain in preview in supported releases. If we're just trying to scare people off from using it in previews, I guess it's ok, but it seems superfluous to me.
There was a problem hiding this comment.
I agree that this would be superfluous for ordinary APIs.
This is codegen intrinsic that may require non-trivial work to complete. Tagging it as preview makes us "ready to ship" without any additional changes, even in the case we do not have a chance to complete the work for some reason. Deleting the tag once the work is complete is a small clean change.
There was a problem hiding this comment.
Fair enough. Of course, we still need to remember to remove the attribute when we complete that additional work. Let's make sure to track that somewhere.
There was a problem hiding this comment.
Updated #82194 to capture it.
|
Could you please also revert the changes in this file: https://github.com/dotnet/runtime/pull/66551/files#diff-80b8cceb2155b097a536baecd12004d4c771062e23633a35e469f0a8920d4f3cR4 |
Done. |
|
Failures seems to be known issue. |
jeffhandley
added
the
blog-candidate
ghost
locked as resolved and limited conversation to collaborators
DivRemAPIs inrefDivRemwith[RequiresPreviewFeatures]