Skip to content
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

Enable nullability in BindingContext and HashKey #7108

Merged
merged 3 commits into from
Nov 22, 2022

Conversation

gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Apr 30, 2022

Proposed changes

  • Enable nullability in BindingContext and HashKey.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner April 30, 2022 06:17
@ghost ghost assigned gpetrou Apr 30, 2022
@ghost ghost added the area: NRT label Apr 30, 2022
@@ -47,7 +44,7 @@ int ICollection.Count
void ICollection.CopyTo(Array ar, int index)
{
ScrubWeakRefs();
_listManagers.CopyTo(ar, index);
new Hashtable(_listManagers).CopyTo(ar, index);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this change. If we use ((ICollection)_listManagers).CopyTo( instead, some tests such as BindingContext_CopyTo_Invoke_Success will fail because the type has changed from DictionaryEntry to KeyValuePair. If modifying the tests is acceptable, then the next issue is that HashKey is private, so we cannot just use that in the assertions. Assuming that we don't just want to make HashKey internal, there might be an alternative approach to achieve what the current assertions do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case updating the test is totally justified

ArrayList cleanupList = null;
foreach (DictionaryEntry de in _listManagers)
List<HashKey> cleanupList = null;
foreach (KeyValuePair<HashKey, WeakReference> de in _listManagers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do deconstruction here? E.g., something like:

Suggested change
foreach (KeyValuePair<HashKey, WeakReference> de in _listManagers)
foreach ((HashKey key, WeakReference wRef) in _listManagers)

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 3, 2022
@elachlan
Copy link
Contributor

Related #8143

Great work with this! I'd love to see it merged.

@RussKie RussKie added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels Nov 11, 2022
@gpetrou gpetrou marked this pull request as draft November 19, 2022 06:11
@ghost ghost added draft draft PR and removed 📭 waiting-author-feedback The team requires more information from the author labels Nov 19, 2022
@gpetrou gpetrou marked this pull request as ready for review November 20, 2022 08:09
@gpetrou
Copy link
Contributor Author

gpetrou commented Nov 22, 2022

@dreddy-work could we also merge this one? #8235 should take care of the ArrayList/Hashtable replacements.

Copy link
Member

@dreddy-work dreddy-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dreddy-work dreddy-work enabled auto-merge (squash) November 22, 2022 23:40
@dreddy-work dreddy-work merged commit 42c26cc into dotnet:main Nov 22, 2022
@ghost ghost added this to the 8.0 Preview1 milestone Nov 22, 2022
@gpetrou gpetrou deleted the BindingContext branch November 23, 2022 16:50
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
draft draft PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants