-
Notifications
You must be signed in to change notification settings - Fork 507
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
[coremedia] Add nullability to (generated and manual) bindings #15066
Conversation
return ((CMCustomBlockAllocator) gch.Target).Allocate (sizeInBytes); | ||
if (gch.Target is CMCustomBlockAllocator target) | ||
return target.Allocate (sizeInBytes); | ||
return IntPtr.Zero; |
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.
Not sure if this is correct
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.
Looks good to me, can't be much worse than throwing a InvalidCastException.
if (retMappingInvalid != IntPtr.Zero) | ||
InvalidMapping = (CMTimeRange)Marshal.PtrToStructure (retMappingInvalid, typeof(CMTimeRange)); | ||
if (retMappingInvalid != IntPtr.Zero) | ||
InvalidMapping = (CMTimeRange)Marshal.PtrToStructure (retMappingInvalid, typeof(CMTimeRange))!; |
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 was also not sure if these castings should be !
. I tried making these CMTimeRange nullable, but I remember Rolf said not to make structs nullable
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.
For both of your questions, I would use the ! since we might be changing behaviour in both cases.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This code does not compile:
Foundation/DictionaryContainer.cs(320,39): error CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
make[1]: *** [build/dotnet/maccatalyst/64/Microsoft.MacCatalyst.dll] Error 1
make[1]: *** Waiting for unfinished jobs....
Foundation/DictionaryContainer.cs(320,39): error CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
make[1]: *** [build/mac/mobile-64/Xamarin.Mac.dll] Error 1
Foundation/DictionaryContainer.cs(320,39): warning CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
Foundation/DictionaryContainer.cs(320,39): warning CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
Foundation/DictionaryContainer.cs(320,39): warning CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
Foundation/DictionaryContainer.cs(320,39): error CS8604: Possible null reference argument for parameter 'dict' in 'CMTime CMTime.FromDictionary(NSDictionary dict)'.
if (retMappingInvalid != IntPtr.Zero) | ||
InvalidMapping = (CMTimeRange)Marshal.PtrToStructure (retMappingInvalid, typeof(CMTimeRange)); | ||
if (retMappingInvalid != IntPtr.Zero) | ||
InvalidMapping = (CMTimeRange)Marshal.PtrToStructure (retMappingInvalid, typeof(CMTimeRange))!; |
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.
For both of your questions, I would use the ! since we might be changing behaviour in both cases.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
return ((CMCustomBlockAllocator) gch.Target).Allocate (sizeInBytes); | ||
if (gch.Target is CMCustomBlockAllocator target) | ||
return target.Allocate (sizeInBytes); | ||
return IntPtr.Zero; |
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.
Looks good to me, can't be much worse than throwing a InvalidCastException.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💻 [PR Build] Tests on macOS Mac Catalina (10.15) passed 💻✅ All tests on macOS Mac Catalina (10.15) passed. Pipeline on Agent |
📋 [PR Build] API Diff 📋API diff (for current PR)ℹ️ API Diff (from PR only) (please review changes) .NETXamarin vs .NETAPI diff (vs stable)✅ API Diff from stable .NETXamarin vs .NETGenerator diff✅ Generator Diff (no change) Pipeline on Agent XAMBOT-1100.Monterey' |
Unrelated Test Failure: https://github.com/xamarin/maccore/issues/2558 |
This PR aims to bring nullability changes to CoreMedia.
Following the steps here:
nullable enable
to all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changes