-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Use GeneratedDllImport in System.Diagnostics.FileVersionInfo and System.Runtime.InteropServices.RuntimeInformation #52739
Conversation
834dbb1
to
1ade806
Compare
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.SYSTEM_INFO.cs
Outdated
Show resolved
Hide resolved
|
||
[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Stat", SetLastError = true)] | ||
internal static extern int Stat(string path, out FileStatus output); | ||
[GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_Stat", CharSet = CharSet.Ansi, SetLastError = true)] |
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.
Why is CharSet = CharSet.Ansi
needed here?
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.
We had the p/invoke source generators avoid making any assumptions about CharSet
if it isn't set, so it requires explicitly specifying either CharSet
or MarshalAs
when marshalling string
/char
.
1ade806
to
00b9f84
Compare
src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetSystemInfo.cs
Outdated
Show resolved
Hide resolved
@@ -8,6 +8,6 @@ internal static partial class Interop | |||
internal static partial class Kernel32 | |||
{ | |||
[DllImport(Libraries.Kernel32)] | |||
internal static extern void GetSystemInfo(out SYSTEM_INFO lpSystemInfo); | |||
internal static unsafe extern void GetSystemInfo(SYSTEM_INFO* lpSystemInfo); |
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.
Is unsafe change necessary? (it is still using the old DllImport)
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.
The parameter was switched to be a pointer - SYSTEM_INFO*
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.
yes but since title is "Use GeneratedDllImport..". this change and changes due to this change looked unrelated.
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.
The general goal of GeneratedDllImport
(and this feature branch) is to remove p/invoke overhead - whether that is through using the DllImport source generator to generate the code or using it to find places where we could easily make the p/invoke inlinable.
System.Diagnostics.FileVersionInfo
:System.Runtime.InteropServices.RuntimeInformation
:cc @AaronRobinsonMSFT @jkoritzinsky