[DllImportGenerator] Remove unnecessary DLLIMPORTGENERATOR_ENABLED ifdefs #61345
Conversation
|
Now that we have down level support, we should consider updating the code fix that uses this preprocessor option. I don’t know if it’s as useful as it was previously. |
|
Agreed - where updating means deleting that option? I want to make sure we get through actually turning on / using the generator for all the down-level runtime libraries first though, and then do that as part of clean-up for productization. |
| @@ -8,13 +8,8 @@ internal static partial class Interop | |||
| { | |||
| internal static partial class Advapi32 | |||
| { | |||
| #if DLLIMPORTGENERATOR_ENABLED | |||
| [GeneratedDllImport(Interop.Libraries.Advapi32, EntryPoint = "ConvertStringSidToSidW", CharSet = CharSet.Unicode, SetLastError = true)] | |||
| [GeneratedDllImport(Interop.Libraries.Advapi32, EntryPoint = "ConvertStringSidToSidW", CharSet = CharSet.Unicode, ExactSpelling = true, SetLastError = true)] | |||
There was a problem hiding this comment.
Was the addition of ExactSpelling here just an opportunistic addition? Should we be using it in other places we're not currently
There was a problem hiding this comment.
Yeah, this was just opportunistic - I just noticed that already had the W suffix while doing this.
| internal static partial int GetWindowsAccountDomainSid( | ||
| #else | ||
| [DllImport(Interop.Libraries.Advapi32, EntryPoint = "GetWindowsAccountDomainSid", CharSet = CharSet.Unicode, SetLastError = true)] | ||
| internal static extern int GetWindowsAccountDomainSid( |
There was a problem hiding this comment.
Do we have an analyzer that warns when specifying a CharSet and it's a nop? I assume that's how you spotted these? But if that's the case, why wasn't it previously firing for this #if case? Or is it just a suggestion right now that doesn't trigger warn-as-error build failures?
There was a problem hiding this comment.
We don't. I just did a quick scan through the changes I had and manually updated a few things.
ghost
locked as resolved and limited conversation to collaborators
Only went through the ones that are in projects that no longer even build down-level.
There's more we should be able to clean up now that we have the forwarding support.
cc @AaronRobinsonMSFT @jkoritzinsky