-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix IList.Contains to work with multidimensional arrays #119583
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1039,7 +1039,22 @@ int IList.Add(object? value) | |||||
|
|
||||||
| bool IList.Contains(object? value) | ||||||
| { | ||||||
| return IndexOf(this, value) >= this.GetLowerBound(0); | ||||||
| // IndexOf only works for single-dimensional arrays. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be fixed in IndexOf instead?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would the output of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered that, but wasn't sure what semantics we would want for IndexOf in terms of the actual index it computes. My intent is to backport this change to 10. If we're ok at this point defining what those semantics would be, I could do that instead.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose the type does implement
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You mean because of the indexer? That throws for MD arrays, too.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thinking was that for 10 we fix Contains in order to fix the LINQ issue, and then for 11 if desired we can look at declaring the ordering and making the other members work... doing so will unblock some other optimizations we previously tried in LINQ but had to back out because the incoming IList wasn't actually usable, and just in general remove unexpected surprises.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That won't work for all MD arrays as they can have more than
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What is the "that" in this statement? IndexOf? That's a good point, too, in addition to ordering, you might have indices too large to return.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any of the |
||||||
| if (Rank == 1) | ||||||
| { | ||||||
| return IndexOf(this, value) >= GetLowerBound(0); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has integer overflow bug. prints true, but it should print false.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we return
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall I fix that in this PR as well? I didn't introduce this line, I just wrapped it in the if.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. It is part of end-to-end of making IList.Contains work correctly for non-SZ arrays. |
||||||
| } | ||||||
|
|
||||||
| // For multi-dimensional arrays, fall back to enumeration. | ||||||
| foreach (object? element in this) | ||||||
| { | ||||||
| if (Equals(element, value)) | ||||||
| { | ||||||
| return true; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| void IList.Clear() | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.