-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.DirectoryServices #61975
Use GeneratedDllImport in System.DirectoryServices #61975
Conversation
ace3523
to
8b3815e
Compare
@@ -436,10 +436,10 @@ public sealed class WKSTA_INFO_100 | |||
}; | |||
|
|||
[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] |
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 the CharSet 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.
Nope.
@@ -7,6 +7,8 @@ | |||
using System.Text; | |||
using System.Diagnostics.CodeAnalysis; | |||
|
|||
using Kernel32 = Interop.Kernel32; |
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 generally don't do this elsewhere. Can we just spell out Interop.Kernel32 at each use site?
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.
I did it because there is also System.DirectoryServices.Interop
and having global::Interop.*
everywhere looked obnoxious to me. I can do global::Interop.*
at each use site if that is preferred.
using Microsoft.Win32.SafeHandles; | ||
|
||
using Advapi32 = Interop.Advapi32; | ||
using Kernel32 = Interop.Kernel32; |
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.
Ditto for all of these.
[DllImport(global::Interop.Libraries.Kernel32, EntryPoint = "GetVersionExW", CharSet = CharSet.Unicode, SetLastError = true)] | ||
internal static extern bool GetVersionEx( | ||
[In, Out] OSVersionInfoEx ver); | ||
[GeneratedDllImport(global::Interop.Libraries.Dnsapi, CharSet = CharSet.Unicode)] |
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 the global:: needed on all of these?
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.
Yeah, I had wondered the same thing - there's a System.DirectoryServices.Interop
, so these all had the global::
internal static partial uint LsaCallAuthenticationPackage( | ||
LsaLogonProcessSafeHandle lsaHandle, | ||
int authenticationPackage, | ||
in NegotiateCallerNameRequest protocolSubmitBuffer, |
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.
How are we choosing when to use 'in' and when to use 'ref'? There was another recent PR where this was discussed and IIRC that one stayed ref
even though it was previously similarly '[In]'.
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.
It has been somewhat opportunistic on our end as a 'could be nice to do' thing where we change it if we notice it would make sense to be in
instead of ref
(in a recent PR, when @jkoritzinsky pointed it out, I did switch the ref
to in
).
src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeLibraryHandle.cs
Show resolved
Hide resolved
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.
LGTM once @stephentoub's feedback is resolved.
I didn't try to move all the existing p/invokes into Common - that can be done separately. I wanted to get through the switch to
GeneratedDllImport
first.@AaronRobinsonMSFT @jkoritzinsky