Avoid setRange with potentially incompatible types #320
Conversation
test/algorithms_test.dart
Outdated
| test('mergeSort works when runtime generic is a subtype of the static type', | ||
| () { | ||
| // Regression test for https://github.com/dart-lang/collection/issues/317 | ||
| final length = 32; // _mergeSortLimit |
There was a problem hiding this comment.
Should probably test above and below the limit - the limit selects a different path.
If the limit is changed, how does this git updated? Perhaps use a value significantly larger.
There was a problem hiding this comment.
Just pick a fairly large number, like 1024.
Whatever the limit is for using insertion-sort, it'll be below that.
There was a problem hiding this comment.
Below the limit should be covered by other tests. This test only needs to ensure that the type error would occur. I'll bump it up to a larger number for safety, I don't think it's worth making the constant public for testing.
test/algorithms_test.dart
Outdated
| test('mergeSort works when runtime generic is a subtype of the static type', | ||
| () { | ||
| // Regression test for https://github.com/dart-lang/collection/issues/317 | ||
| final length = 32; // _mergeSortLimit |
There was a problem hiding this comment.
Just pick a fairly large number, like 1024.
Whatever the limit is for using insertion-sort, it'll be below that.
lib/src/algorithms.dart
Outdated
| @@ -394,8 +394,9 @@ void _merge<E, K>( | |||
| } | |||
| // First list empties first. Reached by break above. | |||
| target[targetOffset++] = secondElement; | |||
| target.setRange( | |||
| targetOffset, targetOffset + (secondEnd - cursor2), secondList, cursor2); | |||
| for (var i = targetOffset; i < targetOffset + (secondEnd - cursor2); i++) { | |||
There was a problem hiding this comment.
Can't _movingInsertionSort above have the same issue?
It contains a setRange call (line 307).
I'd rather change the creation of scratchSpace above to:
var scratchSpace = elements.sublist(0, secondLength);That is guaranteed to create a list of the same kind and type as the original (documented as such), and actually does so for typed-data lists, where it can be a saving.
Or use
var scratchSpace = elements.take(secondLength).toList(growable: false);to at least get a non-growable list (since it's never grown anyway).
There was a problem hiding this comment.
Yes, I missed that there are a couple code paths where this could hit a moving insertion sort. I'm not sure how to construct an input list to exercise that path.
I'll go with the sublist approach to fix them reliably.
lib/src/algorithms.dart
Outdated
| @@ -394,8 +394,9 @@ void _merge<E, K>( | |||
| } | |||
| // First list empties first. Reached by break above. | |||
| target[targetOffset++] = secondElement; | |||
| target.setRange( | |||
| targetOffset, targetOffset + (secondEnd - cursor2), secondList, cursor2); | |||
| for (var i = targetOffset; i < targetOffset + (secondEnd - cursor2); i++) { | |||
There was a problem hiding this comment.
This does extra computation in both condition and assignment.
I predict it would be more efficient as:
for (var i = 0, count = secondEnd - cursor2; i < count; i++) {
target[targetOffset + i] = secondList[cursor2 + i];
}(If these are platform lists, the compiler can make it a mem-move with known offsets and count. Obviously only possible if it's used monomorphically, but how often does someone use mergeSort to begin with? Even if not, less computation inside the inner loop is always good.)
|
I churned below you, @natebosch ! |
|
@rakudrama any concerns about this |
LGTM |
Fixes #317