Skip to content

KF-26 add tests for timsort#58

Open
Kellesi wants to merge 2 commits into
mainfrom
KF-26-test-timsort
Open

KF-26 add tests for timsort#58
Kellesi wants to merge 2 commits into
mainfrom
KF-26-test-timsort

Conversation

@Kellesi

@Kellesi Kellesi commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator

@Kellesi Kellesi requested a review from Copilot August 13, 2025 12:42
@Kellesi Kellesi mentioned this pull request Aug 13, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Kellesi Kellesi requested a review from Copilot August 13, 2025 13:51

This comment was marked as outdated.

@Kellesi Kellesi requested a review from Copilot August 13, 2025 14:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

Adds comprehensive test coverage for the timsort algorithm implementation by creating a new test file with extensive unit tests for all timsort functions.

  • Added a new test file TimsortTest.cpp with 1523 lines of comprehensive test cases
  • Tests cover all utility functions (cmp, clzll, compute_minrun) and core algorithm functions
  • Integrated the test file into the CMake build system

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.

File Description
test/TimsortTest.cpp New comprehensive test suite covering all timsort functions with edge cases and normal operations
test/CMakeLists.txt Added TimsortTest.cpp to the test build configuration
include/kf/algorithm/timsort.h Minor code improvements and bug fixes discovered during testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread test/TimsortTest.cpp
{
std::array<int, 0> arr;

reverse_elements(arr.data(), 0, arr.size() - 1);

Copilot AI Aug 13, 2025

Copy link

Choose a reason for hiding this comment

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

This line will cause undefined behavior when arr.size() is 0. The expression arr.size() - 1 will underflow to a very large number since size() returns an unsigned type. Consider adding a check for empty arrays or using a different approach.

Suggested change
reverse_elements(arr.data(), 0, arr.size() - 1);
// Avoid underflow: only call if array is non-empty
if (arr.size() > 0) {
reverse_elements(arr.data(), 0, arr.size() - 1);
}

Copilot uses AI. Check for mistakes.
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
Comment thread test/TimsortTest.cpp Outdated
@Kellesi

Kellesi commented Aug 13, 2025

Copy link
Copy Markdown
Collaborator Author

Here some functions have unusual or unintuitive usage.
For example, reverse_elements expects end to be the last element instead of the array’s end,
and binary_insertion_sort_start starts at 1 instead of the usual 0.
Might be worth rewriting them in a more standard way to avoid confusion if these functions get used outside of Timsort?

@belyshevdenis

Copy link
Copy Markdown
Collaborator

Here some functions have unusual or unintuitive usage. For example, reverse_elements expects end to be the last element instead of the array’s end, and binary_insertion_sort_start starts at 1 instead of the usual 0. Might be worth rewriting them in a more standard way to avoid confusion if these functions get used outside of Timsort?

Definitely we should fix the logical mistakes in code using logical tests, instead cover illogical functionality.

@Kellesi

Kellesi commented Aug 15, 2025

Copy link
Copy Markdown
Collaborator Author

I've fixed small issues and incorrect behavior, but fixing the full logic would take more time because all these functions depend on each other, and I guess I also need approval to do this. And another task for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants