Skip to content
This repository has been archived by the owner on Oct 22, 2024. It is now read-only.

Avoid setRange with potentially incompatible types #320

Merged
merged 5 commits into from
Nov 21, 2023
Merged

Avoid setRange with potentially incompatible types #320

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
223033d
Avoid setRange with potentially incompatible types
natebosch Nov 1, 2023
d75e68b
Larger for safety
natebosch Nov 8, 2023
0502a0f
Merge branch 'master' into merge-type
natebosch Nov 9, 2023
e3149c6
Use sublist to get correct type
natebosch Nov 9, 2023
ed0f6d0
use and unsorted list
natebosch Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

- Adds `shuffled` to `IterableExtension`.
- Shuffle `IterableExtension.sample` results.
- Fix `mergeSort` when the runtime iterable generic is a subtype of the static
generic.

## 1.18.0

Expand Down
5 changes: 3 additions & 2 deletions lib/src/algorithms.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.)

target[i] = secondList[(i - targetOffset) + cursor2];
}
}

/// Sort [elements] using a quick-sort algorithm.
Expand Down
9 changes: 9 additions & 0 deletions test/algorithms_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,15 @@ void main() {
reverse(l, 0, 6);
expect(l, equals([2, 1, 4, 3, 6, 5]));
});

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
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just pick a fairly large number, like 1024.
Whatever the limit is for using insertion-sort, it'll be below that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// In order list, first half empties first during merge.
final list = List<int>.generate(length, (i) => i);
natebosch marked this conversation as resolved.
Show resolved Hide resolved
expect(() => mergeSort<num>(list), returnsNormally);
});
}

class C {
Expand Down