Skip to content

KF-26 add tests for timsort#58

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

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

Conversation

@Kellesi
Copy link
Collaborator

@Kellesi Kellesi commented Aug 13, 2025

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

Copilot AI left a comment

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

Copilot AI left a comment

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.

{
std::array<int, 0> arr;

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

Copilot AI Aug 13, 2025

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.
@Kellesi
Copy link
Collaborator Author

Kellesi commented Aug 13, 2025

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

Kellesi commented Aug 15, 2025

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