-
-
Notifications
You must be signed in to change notification settings - Fork 228
Conversation
qmfrederik
commented
Jun 29, 2020
- SetupDiCreateDeviceInfoList
- SetupDiOpenDeviceInfo
- SetupDiSetSelectedDevice
- SetupDiGetDeviceInstallParams
- SetupDiSetDeviceInstallParams
- SetupDiBuildDriverInfoList
- SetupDiEnumDriverInfo
- SetupDiSetSelectedDriver
- SetupDiCreateDeviceInfoList - SetupDiOpenDeviceInfo - SetupDiSetSelectedDevice - SetupDiGetDeviceInstallParams - SetupDiSetDeviceInstallParams - SetupDiBuildDriverInfoList - SetupDiEnumDriverInfo - SetupDiSetSelectedDriver
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.
Thanks for contributing! I have several comments but will be happy to merge when they're resolved.
src/SetupApi/SetupApi.cs
Outdated
/// <seealso href="https://msdn.microsoft.com/en-us/library/windows/hardware/ff550956(v=vs.85).aspx"/> | ||
[DllImport(nameof(SetupApi), SetLastError = true)] | ||
public static unsafe extern SafeDeviceInfoSetHandle SetupDiCreateDeviceInfoList( | ||
Guid* classGuid, |
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.
Add [Friendly(FriendlyFlags.In | FriendlyFlags.Optional)]
on this parameter so that we get friendly overloads including (I suspect) in Guid
so that you can naturally pass a GUID in, while still retaining the ability to pass in null
on this overload.
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.
Thanks, I've added these flags. You now get overloads for Guid*
and Guid?
, which means that you need to cast null
to either of these types.
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.
which means that you need to cast null to either of these types.
Ya, that's unfortunate, but not exactly a new problem either in general.
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.
nit: style-wise we define a parameter with its attribute on one line.
src/SetupApi/SetupApi.cs
Outdated
string deviceInstanceId, | ||
IntPtr parent, | ||
OpenFlags openFlags, | ||
ref SP_DEVINFO_DATA deviceInfoData); |
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 doc indicates this last parameter is optional and can be null. Can you please change this parameter to:
[Friendly(FriendlyFlags.Bidirectional | FriendlyFlags.Optional)] SP_DEVINFO_DATA*
I believe we'll still generate a ref Struct
overload, but folks can then also pass null
as an argument instead.
BTW, this won't work if SP_DEVINFO_DATA
has MarshalAs
attributed fields on it, which is why we also discourage those attributes in structs, also mentioned in our contributing doc. :)
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 is not optional, so I've changed the argument to [Friendly(FriendlyFlags.Bidirectional)] SP_DEVINFO_DATA*
src/SetupApi/SetupApi.cs
Outdated
ref SP_DEVINFO_DATA devceInfoData, | ||
ref SP_DEVINSTALL_PARAMS deviceInstallParams); |
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 suspect similar corrections to what I describe above apply here.
Thanks, @AArnott. I think I've addressed the first round of feedback, let me know if you have further comments. |
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.
Very close. Thanks again.
src/SetupApi/SetupApi.cs
Outdated
/// <seealso href="https://msdn.microsoft.com/en-us/library/windows/hardware/ff550956(v=vs.85).aspx"/> | ||
[DllImport(nameof(SetupApi), SetLastError = true)] | ||
public static unsafe extern SafeDeviceInfoSetHandle SetupDiCreateDeviceInfoList( | ||
Guid* classGuid, |
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.
nit: style-wise we define a parameter with its attribute on one line.
@AArnott Allright, looks like CI is green on this one. |