-
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
[webkit] Add nullability to (generated and manual) bindings #15028
Conversation
@@ -24,23 +26,25 @@ internal class IndexedContainerEnumerator<T> : IEnumerator<T> { | |||
|
|||
public T Current { | |||
get { | |||
return _container [_index]; | |||
return _container! [_index]; | |||
} |
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.
Highlighting this !
. The return type cannot be made nullable since it is implementing an interface with that method signature but it is also a getter that is not an indexer so I thought this may be the best option
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.
what about using:
if (_container is null) {
return default (T);
} else {
return _container [_index]
}
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.
Might make more sense to throw an ObjectDisposedException
.
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.
@rolfbjarne good one, that exception should also be a good idea for my other comment.
return ++_index < _container.Count; | ||
} | ||
|
||
public void Reset () { | ||
_index = -1; | ||
} | ||
|
||
IIndexedContainer<T> _container; | ||
IIndexedContainer<T>? _container; |
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.
Why is this needed, if the only constructor takes a non-nullable container
value and assigns it to this field?
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.
@rolfbjarne I added this, because the Dispose method assigns null to _container
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.
@rolfbjarne we need a way to init that. The compiler is complaining because the default value of a IIndexedContainer is null. We could probably make the _container a private Init property with the type and remove the need of ? and remove all the Dipose code. Should make things nicer.
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.
@rolfbjarne @mandel-macaque Should I do this?
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.
@tj-devel709 yes, keep this since nulling it the field in the Dispose method might be important
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.
@@ -24,23 +26,25 @@ internal class IndexedContainerEnumerator<T> : IEnumerator<T> { | |||
|
|||
public T Current { | |||
get { | |||
return _container [_index]; | |||
return _container! [_index]; | |||
} |
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.
what about using:
if (_container is null) {
return default (T);
} else {
return _container [_index]
}
src/WebKit/Enumerators.cs
Outdated
if (_container is null) | ||
return false; |
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 is not correct. MoveNext can throw and throws and exception, please read https://docs.microsoft.com/en-us/dotnet/api/system.collections.ienumerator.movenext?view=net-6.0
Returning false has a meaning:
true if the enumerator was successfully advanced to the next element; false if the enumerator has passed the end of the collection.
That means that you are telling the user of the API that everything was ok and that you reached the end, that is not the case. We should throw an InvalidOperationException.
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.
_container
can only be null if the object has been disposed, so an ObjectDisposedException
sounds natural
return ++_index < _container.Count; | ||
} | ||
|
||
public void Reset () { | ||
_index = -1; | ||
} | ||
|
||
IIndexedContainer<T> _container; | ||
IIndexedContainer<T>? _container; |
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.
@rolfbjarne we need a way to init that. The compiler is complaining because the default value of a IIndexedContainer is null. We could probably make the _container a private Init property with the type and remove the need of ? and remove all the Dipose code. Should make things nicer.
@@ -24,23 +26,25 @@ internal class IndexedContainerEnumerator<T> : IEnumerator<T> { | |||
|
|||
public T Current { | |||
get { | |||
return _container [_index]; | |||
return _container! [_index]; | |||
} |
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.
Might make more sense to throw an ObjectDisposedException
.
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.
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.
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes).NET (No breaking changes)❗ API diff vs stable (Breaking changes)Legacy Xamarin (No breaking changes).NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
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.
🔥 Unable to find the contents for the comment: D:\a\1\s\change-detection\results\gh-comment.md does not exist :fire Pipeline on Agent |
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.
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
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 |
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
❌ [CI Build] Tests failed on VSTS: simulator tests iOS ❌Tests failed on VSTS: simulator tests iOS. Test results1 tests failed, 147 tests passed.Failed tests
Pipeline on Agent XAMBOT-1044.Monterey' |
Unrelated Test Failure: https://github.com/xamarin/maccore/issues/2558 |
This PR aims to bring nullability changes to WebKit.
Following the steps here:
nullable enable
to 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== null
or!= null
tois null
andis not null