-
Notifications
You must be signed in to change notification settings - Fork 358
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
Updating GenAPI to properly handle nint for C# 9 and later #9421
Conversation
{ | ||
if (TypeHelper.TypesAreEquivalent(type, type.PlatformType.SystemIntPtr)) | ||
{ | ||
if ((LangVersion >= LangVersion11_0) || type.Attributes.HasNativeIntegerAttribute()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to C# 11, IntPtr
was IntPtr
and nint
was [NativeInteger] IntPtr
.
C# 11 and later, its just nint
provided at least .NET 7 is targeted. Since C# 11 is only "officially" supported on .NET 7 and later, GenAPI can just treat this "synonymously".
dotnet/runtime#69627 is the runtime side change that picks up the compiler changes |
I kindly ask @ericstj to review this change, as it touches the CCI codebase. |
I'd like someone from @dotnet/area-infrastructure-libraries review the code for correctness and provide signoff. Consider testing methods like those previously suggested in #8769 (comment) and share the details for how you tested this to ensure the changes work as expected without regressions. |
@ericstj, I validated this against dotnet/runtime as always. The change is simply that on .NET 7 and later |
Ping @dotnet/area-infrastructure-libraries could this get a review and merge? |
@ViktorHofer could I get a sign-off on this one as well? |
I asked @carlossanlop to take a look here as he recently touched the cci code base as well. I myself never worked on that library so I would like to defer to Carlos. |
Based on @tannergooding's comment, the reference sources in dotnet/runtime are already generated based on these changes so we should get this in asap. Merging. |
CC. @ViktorHofer, @ericstj