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

ColList: preserve list order on list.push(..) #1474

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jul 2, 2024

Description of Changes

Preserve the list order of ColList for e.g., col_list![2, 0] by heapifying in those cases.
This might not be the optimal performance solution, but this fixes the bug while preserving that size_of::<ColList> == 8 and that it is still inline in the common case.

API and ABI breaking changes

Rather unlikely that anyone relied on this bug.

Expected complexity level and risk

2, some unsafe and someone might have relied on this bug.

Testing

  • Tests are amended to ensure that the list order is preserved.

@Centril Centril requested a review from kazimuth July 2, 2024 11:00
@Centril Centril force-pushed the centril/col-actual-list branch from d0d1445 to fa56b2b Compare July 5, 2024 10:50
@bfops bfops added release-any To be landed in any release window bugfix Fixes something that was expected to work differently labels Jul 8, 2024
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Ha! Brilliant. LGTM 👍

(I was going to suggest sorting inputs in the FromIterator impl, but then I realized that would defeat the purpose of this PR... it would make sense if you were implementing From<HashSet>, but there's no iterator equivalent of that.)

@Centril Centril added this pull request to the merge queue Jul 8, 2024
Merged via the queue into master with commit cbff725 Jul 8, 2024
8 checks passed
lcodes pushed a commit that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes something that was expected to work differently release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants