Fix behaviour of TreeSubSet Min and Max #16771
Conversation
Not without an API review. cc: @terrajobst public T Min => MinInternal;
internal virtual T MinInternal { get { /* code previously in Min */ } }and then override MinInternal in TreeSubSet (and the same for Max)? I'm not very familiar with the situations in which Min and Max would be used. Is an extra virtual call on such code paths going to be problematic for performance? |
|
A change here would require test coverage. |
#16763 (this PR needs to be rebased to remove the [ActiveIssue]) |
mjp41
force-pushed
the
SortedSet-fix
branch
3 times, most recently
from
2b0812c to
a95f794
Compare
|
I rebased on top of the tests, and made it use an Internal virtual call. |
|
Shouldn't I see the [ActiveIssue] being removed? |
It is. |
|
I have extended with a more exhaustive test of a small tree, so that I believe all the corner cases should be covered now. |
Provide correct override in TreeSubSet for Min and Max, they now take account of the range of the subset. As the root is guaranteed to be in the range, then the code does not need to check something is in the range, if _root is not null.
|
@dotnet-bot test code coverage please |
|
|
||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
Nits:
- Could you move the opening braces from the end of the lines to the beginning of the next?
- Could you move the else to its own line?
- Could you surround the break with braces (on their own lines)?
Same in MaxInternal below.
| } else { | ||
| Assert.Equal(i + ((i+1) % 2), view.Min); | ||
| Assert.Equal(j - ((j+1) % 2), view.Max); | ||
| } |
There was a problem hiding this comment.
Nit: same style nits, e.g. braces
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Thanks for fixing this, @mjp41.
|
@stephentoub thanks for the feedback. I have addressed the comments. Do you want me to collapse to a single commit? |
|
Thanks, @mjp41. No need; I'll just squash it when merging once CI is green. |
This reverts commit d467d43.
* Fix behaviour of TreeSubSet Min and Max Provide correct override in TreeSubSet for Min and Max, they now take account of the range of the subset. As the root is guaranteed to be in the range, then the code does not need to check something is in the range, if _root is not null. * Added a more exhaustive test of Min/Max on TreeSubSet. Commit migrated from dotnet/corefx@d467d43
7 participants
Fixes #16760.
Provide correct override in TreeSubSet for Min and Max, they now take
account of the range of the subset.
As the root is guaranteed to be in the range, then the code does not
need to check something is in the range, if _root is not null.
This makes Min and Max virtual. Is that an acceptable change? Previously, they hid the virtual behaviour inside the implementation.
cc @stephentoub @marek-safar @ianhays