[compression] Add nullability to (generated and manual) bindings - #14916
Conversation
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.
| Inflater Inflater { | ||
| get { | ||
| if (_inflater is null) | ||
| _inflater = new Inflater (_algorithm); |
There was a problem hiding this comment.
It looks more appropriate to throw an exception here:
| _inflater = new Inflater (_algorithm); | |
| throw new InvalidOperationException ("Can't access an inflater when decompressing"); |
There was a problem hiding this comment.
I like the idea, since we should have init the inflater in the constructor, and we can get the nullability check or throw. Using this + the property trick makes the clone simpler to understand.
| Deflater Deflater { | ||
| get { | ||
| if (_deflater is null) | ||
| _deflater = new Deflater (_algorithm); |
There was a problem hiding this comment.
| _deflater = new Deflater (_algorithm); | |
| throw new InvalidOperationException ("Can't access a deflater when compressing"); |
| Byte[] Buffer { | ||
| get { | ||
| if (_buffer is null) | ||
| _buffer = ArrayPool<byte>.Shared.Rent (DefaultBufferSize); |
There was a problem hiding this comment.
| _buffer = ArrayPool<byte>.Shared.Rent (DefaultBufferSize); | |
| throw new InvalidOperationException ("Buffer has not been initialized"); |
and keep EnsureBufferInitialized.
Automatically initializing can sometimes run into problems where the field is cleared, and then inadvertently re-created, which causes for some hard-to-find bugs. It's easier if an exception is thrown in that scenario.
This comment has been minimized.
This comment has been minimized.
| Inflater Inflater { | ||
| get { | ||
| if (_inflater is null) | ||
| _inflater = new Inflater (_algorithm); |
There was a problem hiding this comment.
I like the idea, since we should have init the inflater in the constructor, and we can get the nullability check or throw. Using this + the property trick makes the clone simpler to understand.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
|
@rolfbjarne ready for re-review! |
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.
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.
|
There are some important test failures that I need to address here! |
âĶo Nullable-Compression-WithManuel
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] Tests on macOS M1 - Mac Big Sur (11.5) failed âFailed tests are:
Pipeline on Agent |
ð [PR Build] API Diff ðAPI Current PR diffâđïļ API Diff (from PR only) (please review changes) View dotnet API diffView dotnet legacy API diffAPI diffâ API Diff from stable View dotnet API diffView dotnet legacy API diffGenerator diffâ Generator Diff (no change) Pipeline on Agent XAMBOT-1108.Monterey' |
5 participants
This PR aims to bring nullability changes to Compression.
Following the steps here:
nullable enableto all manual files that are not "API_SOURCES" in src/frameworks.sources and making the required nullability changesthrow new ArgumentNullException ("object"));toObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (object));for size saving optimization as well to mark that this framework contains nullability changes== nullor!= nulltois nullandis not nullI am replacing this older PR: #14408
with this one! @mandel-macaque helped me through this process making this API a little cleaner than my previous PR!