Silence some marshalling warnings #690
Conversation
* Call into PInvokeMarshal from the generated marshallers. These are all analyzed at compile time and safe. Calling directly into implementation sidesteps the warnings and provides a mild performance boost. Only the `GetFunctionPointerForDelegate` part is actually needed. The other was done for consistency. * Silence warnings on generic things. I didn't see this before because these are generic and they weren't instantiated anywhere I was looking at.
MichalStrehovsky
added
the
area-NativeAOT-coreclr
| @@ -582,6 +584,8 @@ public static void PtrToStructure<T>(IntPtr ptr, [DisallowNull] T structure) | |||
| PtrToStructure(ptr, (object)structure!); | |||
There was a problem hiding this comment.
Do we need UnconditionalSuppressMessage on void PtrToStructure<T>(IntPtr ptr, [DisallowNull] T structure) too?
| @@ -582,6 +584,8 @@ public static void PtrToStructure<T>(IntPtr ptr, [DisallowNull] T structure) | |||
| PtrToStructure(ptr, (object)structure!); | |||
| } | |||
|
|
|||
| [UnconditionalSuppressMessage("AotAnalysis", "IL9700:AotUnfriendlyApi", | |||
| Justification = "AOT compilers can see the T.")] | |||
There was a problem hiding this comment.
I am wondering whether it would be better to call PtrToStructureHelper instead of suppressing the warning. Some of the argument checks would need to be duplicated, but we would be also able to omit some.
There was a problem hiding this comment.
(similar to other places like GetDelegateForFunctionPointer)
| throw new ArgumentException(SR.Arg_MustBeDelegate, nameof(TDelegate)); | ||
| } | ||
|
|
||
| return (TDelegate)(object)GetDelegateForFunctionPointerInternal(ptr, t); | ||
| } | ||
|
|
||
| [RequiresDynamicCode("Marshalling code for the delegate might not be available. Use the GetFunctionPointerForDelegate<TDelegate> overload instead.")] |
There was a problem hiding this comment.
Do we also need to update public static IntPtr GetFunctionPointerForDelegate<TDelegate>(TDelegate d) where TDelegate : notnull to not produce warning?
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs
Outdated
Show resolved
Hide resolved
| throw new ArgumentException(SR.Arg_MustBeDelegate, nameof(TDelegate)); | ||
| } | ||
|
|
||
| return (TDelegate)(object)GetDelegateForFunctionPointerInternal(ptr, t); | ||
| } | ||
|
|
||
| [RequiresDynamicCode("Marshalling code for the delegate might not be available. Use the GetFunctionPointerForDelegate<TDelegate> overload instead.")] |
There was a problem hiding this comment.
Nit: I just tried this and noticed that there is a double-dot printed in the warning message Use the GetFunctionPointerForDelegate<TDelegate> overload instead... Should we stop appending . to the message in ReflecitonMethodBodyScanner.cs?
There was a problem hiding this comment.
I think we'll never be consistent with this. It might be better to just fix this up when the message is generated (append the dot only if it's not there).
GetFunctionPointerForDelegatepart is actually needed. The other was done for consistency.