Feature/add sort #35
Feature/add sort #35
Conversation
WalkthroughThis pull request introduces a new sorting module with its associated test file and integrates them into the project’s build system via CMake. In addition, the update enhances cross-platform support by incorporating a new macro for stack allocation and performs widespread copyright updates to reflect the current year. Minor formatting and declaration adjustments were also applied to some test files. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as SortTest
participant Sort as Sort.h
participant Alloc as CPPCORE_STACK_ALLOC
Test->>Sort: Call quicksort(data, num, stride, comp)
Sort->>Alloc: Allocate memory for pivot
Sort->>Sort: Invoke quicksortImpl recursively
Sort-->>Test: Return sorted data
Test->>Sort: Call binSearch(key, sorted_array, num, comp)
Sort-->>Test: Return index or error code
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
include/cppcore/Common/TStringBase.h (1)
189-190:⚠️ Potential issueIncomplete Implementation:
TStringView::data()
The implementation of thedata()method inTStringViewis currently empty and does not return a value. To ensure correct functionality and avoid undefined behavior, it should return the member pointermPtr.Suggested change:
-inline TCharType *TStringView<TCharType>::data() const { -} +inline TCharType *TStringView<TCharType>::data() const { + return mPtr; +}include/cppcore/Common/TOptional.h (1)
107-117:⚠️ Potential issueIncorrect Member Access in
operator==
Within theoperator==implementation, the code comparesmInitedwithrhs.isInitedinstead of comparing against themInitedmember variable (or by invoking the accessor function with parentheses). This likely leads to an unintended function pointer comparison. It is recommended to update the comparison as shown below.Suggested change:
- if (mInited == rhs.isInited) { + if (mInited == rhs.mInited) {include/cppcore/Container/TQueue.h (1)
138-146:⚠️ Potential issueTypo in Assignment Operator
Within the operator= implementation, the code usesrhs.m_QueueData(line 143) whereas the member is declared asmQueueData. This mismatch will likely cause compile errors. Please updaterhs.m_QueueDatatorhs.mQueueData.include/cppcore/Memory/TPoolAllocator.h (1)
202-212: 🛠️ Refactor suggestionPotential Memory Leak in reserve() Function.
Within thereserve()method, the code first creates a new pool via thePoolconstructor—which already allocates memory formPool—and then immediately assigns a new allocation tomCurrent->mPool, overwriting the original pointer. This redundancy may result in a memory leak.Consider removing the redundant allocation lines. For example, update the function as follows:
- mCurrent->mPool = new T[size]; - mCurrent->mPoolsize = size;since the
Pool(size_t, Pool*)constructor already handles the allocation.
🧹 Nitpick comments (10)
include/cppcore/Common/TBitField.h (1)
100-103: Enhance Bit Shift Expression
In thegetBitmethod, consider replacing1 << poswithstatic_cast<T>(1) << posto ensure correct type handling for different instantiations ofT. This change would improve the robustness of the bitwise operations whenTis not an int.test/container/TListTest.cpp (1)
122-156: Test Coverage Enhancement Suggestion:
The tests in this file provide good coverage for the TList functionalities (construction, add, copy, and iteration). However, there is currently no test for theremoveBack()method. Adding unit tests forremoveBack()would improve coverage and help ensure that all path functionalities of TList are validated.include/cppcore/Memory/TStackAllocator.h (2)
127-150: Allocator Memory Allocation Implementation
Theallocfunction correctly calculates the required memory (including header overhead) and updates internal state accordingly.Suggestion: For improved type-safety and clarity, consider replacing the C-style cast on line 140 with
reinterpret_cast. For example:- Header *header = (Header *)(&mData[mTop]); + Header *header = reinterpret_cast<Header*>(&mData[mTop]);
219-220: Namespace Comment Consistency
The closing comment for the namespace shows a typo. It currently reads:} // Namespace cppcorecppcoreConsider correcting it to match the declared namespace:
- } // Namespace cppcorecppcore + } // Namespace cppcoretest/io/FileSystemTest.cpp (1)
35-39: Hard-Coded Filesystem Path Warning.
The test case uses the hard-coded path"c:\\"to obtain free disk space. This could cause issues on non-Windows platforms. Consider parameterizing the filesystem root or using a platform-independent directory for testing to ensure cross-platform compatibility.include/cppcore/CPPCoreCommon.h (2)
32-32: Consider using<stdlib.h>instead of<malloc.h>.While
<malloc.h>provides the necessary declarations foralloca, it's considered obsolete in some systems.<stdlib.h>is more portable and standard-compliant.-#include <malloc.h> +#include <stdlib.h>
57-57: Add safety warnings for the stack allocation macro.The
CPPCORE_STACK_ALLOCmacro is a good addition for cross-platform support, but stack allocations can be dangerous:
- Stack space is limited, and large allocations may cause stack overflow
- No size checks are performed before allocation
- Allocations don't check for failure
Consider adding a maximum size check or documentation warning.
Also applies to: 60-60
test/common/SortTest.cpp (1)
53-58: Verify sorting order inquicksortTest.The test sorts the array using
compDescendingbut then verifies it's sorted using the samecompDescendingfunction. This will always pass regardless of whether the sort works correctly. Consider using different comparison functions for sorting and verification.- quicksort(arr, 5, sizeof(int32_t), compDescending<int32_t>); - bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>); + quicksort(arr, 5, sizeof(int32_t), compAscending<int32_t>); + bool sorted = isSorted(arr, 5, sizeof(int32_t), compAscending<int32_t>);include/cppcore/Common/Sort.h (2)
31-36: Improve readability of comparison function.The comparison expression
(_lhs > _rhs) - (_lhs < _rhs)is clever but not immediately obvious. Consider using a more explicit implementation for better readability.template<class T> inline int32_t compAscending(const void *lhs, const void *rhs) { const T _lhs = *static_cast<const T *>(lhs); const T _rhs = *static_cast<const T *>(rhs); - return (_lhs > _rhs) - (_lhs < _rhs); + if (_lhs < _rhs) return -1; + if (_lhs > _rhs) return 1; + return 0; }
65-95: Improve quicksort pivot selection to avoid worst-case performance.The current implementation always uses the first element as the pivot, which can lead to O(n²) performance for already-sorted arrays. Consider implementing median-of-three or randomized pivot selection.
inline void quicksortImpl(void *pivot, void *_data, size_t num, size_t stride, ComparisonFn func) { if (num < 2) { return; } if (_data == nullptr) { return; } uint8_t *data = (uint8_t*) _data; - memcpy(pivot, &data[0], stride); + // Use median-of-three pivot selection to avoid worst-case behavior + size_t mid = num / 2; + size_t last = num - 1; + // Swap middle element with first if necessary + if (func(&data[0], &data[mid * stride]) > 0) { + swap(&data[0], &data[mid * stride], stride); + } + // Swap first with last if necessary + if (func(&data[0], &data[last * stride]) > 0) { + swap(&data[0], &data[last * stride], stride); + } + // Swap middle with last if necessary + if (func(&data[mid * stride], &data[last * stride]) > 0) { + swap(&data[mid * stride], &data[last * stride], stride); + } + // Use middle element as pivot + memcpy(pivot, &data[mid * stride], stride);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
CMakeLists.txt(2 hunks)code/cppcore.cpp(1 hunks)include/cppcore/CPPCoreCommon.h(2 hunks)include/cppcore/Common/Sort.h(1 hunks)include/cppcore/Common/TBitField.h(1 hunks)include/cppcore/Common/TOptional.h(1 hunks)include/cppcore/Common/TStringBase.h(1 hunks)include/cppcore/Common/Variant.h(1 hunks)include/cppcore/Container/TArray.h(1 hunks)include/cppcore/Container/THashMap.h(1 hunks)include/cppcore/Container/TList.h(1 hunks)include/cppcore/Container/TQueue.h(1 hunks)include/cppcore/Container/TStaticArray.h(1 hunks)include/cppcore/Memory/MemUtils.h(1 hunks)include/cppcore/Memory/TDefaultAllocator.h(1 hunks)include/cppcore/Memory/TPoolAllocator.h(1 hunks)include/cppcore/Memory/TStackAllocator.h(1 hunks)include/cppcore/Random/RandomGenerator.h(1 hunks)test/CPPCoreCommonTest.cpp(1 hunks)test/common/HashTest.cpp(1 hunks)test/common/SortTest.cpp(1 hunks)test/common/TBitFieldTest.cpp(1 hunks)test/common/TOptionalTest.cpp(1 hunks)test/common/VariantTest.cpp(1 hunks)test/container/TArrayTest.cpp(1 hunks)test/container/THashMapTest.cpp(2 hunks)test/container/TListTest.cpp(1 hunks)test/container/TQueueTest.cpp(1 hunks)test/container/TStaticArrayTest.cpp(1 hunks)test/io/FileSystemTest.cpp(1 hunks)test/memory/TPoolAllocatorTest.cpp(3 hunks)test/memory/TScratchAllocatorTest.cpp(2 hunks)test/memory/TStackAllocatorTest.cpp(1 hunks)test/random/RandomGeneratorTest.cpp(1 hunks)
🔇 Additional comments (39)
include/cppcore/Common/TStringBase.h (1)
4-4: Textual Update: Copyright Year
The copyright notice has been updated to reflect the period 2014-2025. This is a purely legal/textual change and is consistent with similar updates across the codebase.code/cppcore.cpp (1)
1-5: Textual Update: Copyright Year
The license header has been updated with the new period (2014-2025). This update is purely cosmetic and does not affect any functionality.include/cppcore/Container/TStaticArray.h (1)
1-4: Textual Update: Copyright Year
According to the PR objectives, the header comment has been updated from "2014-2024" to "2014-2025". This change is purely textual and is applied consistently with other files.include/cppcore/Common/TOptional.h (1)
4-4: Textual Update: Copyright Year
The header has been updated to reflect the new copyright period (2014-2025). No functional changes are involved.test/common/TOptionalTest.cpp (1)
1-5: Textual Update: Copyright Year
The header comment has been updated to the current period (2014-2025). This change is purely textual and does not impact the functionality of the tests.test/container/TQueueTest.cpp (1)
5-5: Updated Copyright Notice
The copyright year has been updated from 2024 to 2025, which is consistent with other files.include/cppcore/Container/TQueue.h (1)
4-4: Updated Copyright Notice
The license header now reflects the updated copyright year (2025).include/cppcore/Common/TBitField.h (1)
4-4: Updated Copyright Notice
The license header update to include the current year (2025) is correctly applied.test/container/TArrayTest.cpp (2)
5-5: Updated Copyright Notice
The license header has been updated to reflect the new year (2025), ensuring consistency across the project.
59-75: Comprehensive Array Test Coverage
The test cases within this file thoroughly validate array construction, element access, modification, iteration, and removal. The use of descriptive messages and varied scenarios helps ensure that the TArray implementation is robust.test/container/TStaticArrayTest.cpp (1)
5-5: Updated Copyright Notice
The license header now correctly reflects the current year (2025), which aligns with the recent updates across the codebase.test/common/HashTest.cpp (1)
1-10: License Header Update:
The license comment has been updated to reflect the current year (2025). This change is consistent with other parts of the codebase and does not impact functionality.include/cppcore/Container/TList.h (1)
1-7: License Header Consistency:
The copyright notice is now updated to “2014-2025”. This update improves consistency across the codebase with no side effects on container functionality.test/CPPCoreCommonTest.cpp (1)
1-10: Updated Copyright Notice:
The MIT License header now reflects the year 2025, ensuring that all components of the project are up to date.include/cppcore/Memory/TDefaultAllocator.h (1)
1-10: License Header Update:
The license header has been updated to “2014-2025”, maintaining uniformity with the rest of the project. No functional changes have been introduced.test/container/TListTest.cpp (1)
1-8: License Header Update in Tests:
The copyright notice in the license header is now updated to “2014-2025”. This minor change enhances consistency across testing files.include/cppcore/Random/RandomGenerator.h (1)
1-6: License Header Update
The copyright header has been updated correctly from "2014-2024" to "2014-2025". There are no functional changes in this file, and the update helps maintain legal accuracy and consistency with the current year.include/cppcore/Container/THashMap.h (1)
1-7: Consistent Copyright Update
The license header now reflects "2014-2025" instead of the previous year. This textual update is consistent with other project files and does not impact any functionality.test/memory/TStackAllocatorTest.cpp (3)
1-7: License and Header Validation
The license header is updated correctly along with the inclusion of necessary headers and namespace usage. This ensures consistency with the rest of the project.
29-39: Allocation and Exception Safety Tests
TheCreateTestensures that the allocator can be constructed without exceptions. The test structure is clear and well-organized.
41-60: Comprehensive Allocation and Release Testing
The tests cover key scenarios for the stack allocator:
- Valid allocation and memory usage reduction.
- Correct restoration of free memory upon release.
- Handling of bad allocations and releases.
This provides a good level of coverage for basic functionality.test/common/TBitFieldTest.cpp (2)
1-8: License and Setup Check
The header license has been updated appropriately, and the required headers and namespace declarations are intact.
29-77: TBitField Functionality Testing
The test cases for creating a bitfield, manipulating bits (set, clear), and verifying the maximum number of bits for various types are well-structured and comprehensive.include/cppcore/Memory/TStackAllocator.h (2)
1-7: License Header Update
The header license has been updated to reflect "2014-2025", ensuring legal and historical consistency across files.
153-170: Release Function Verification
Thereleasemethod properly locates and subtracts the allocated block (including its header) from the top pointer. It correctly decrements the allocation counter.Ensure that in scenarios with misordered releases (which are not supported), the caller is aware that the behavior is undefined.
include/cppcore/Common/Variant.h (1)
4-4: Update Copyright Notice in Variant.h.
The copyright notice has been updated to reflect the current year (2025). This is a purely textual change that maintains consistency without affecting functionality.test/common/VariantTest.cpp (1)
4-4: Update Copyright Notice in VariantTest.cpp.
The copyright header is updated consistently to 2025. No functional changes are introduced.include/cppcore/Memory/MemUtils.h (1)
4-4: Update Copyright Notice in MemUtils.h.
The copyright notice has been updated from "2014-2024" to "2014-2025", which aligns with the other project files. There are no changes to functionality.test/random/RandomGeneratorTest.cpp (1)
4-4: LGTM: Copyright year updated.The copyright year has been updated to reflect 2025.
include/cppcore/Container/TArray.h (1)
4-4: LGTM: Copyright year updated.The copyright year has been updated to reflect 2025.
CMakeLists.txt (2)
80-80: New Sort.h header file integration looks good.The Sort.h header has been properly added to the common source files, enabling the new sorting functionality to be included in the build.
137-137: New SortTest.cpp integration looks good.The SortTest.cpp file has been properly added to the common test files, ensuring the new sorting functionality will be tested through the unit test framework.
test/container/THashMapTest.cpp (2)
5-5: LGTM: Copyright year updated.The copyright year has been updated to reflect 2025.
31-31: Code style improvement.The class declaration has been simplified by removing the redundant
// emptycomment while preserving functionality.test/memory/TPoolAllocatorTest.cpp (3)
4-4: LGTM: Copyright year updated.The copyright year has been updated to reflect 2025.
37-37: LGTM: Code formatting update.The constructor has been reformatted to a single line, improving readability.
52-52: Good modernization: Using constexpr instead of const.Changing from
static consttostatic constexpris a good practice as it enables compile-time evaluation and potential optimizations.test/memory/TScratchAllocatorTest.cpp (2)
4-4: Copyright year updated to reflect 2025.The copyright year has been updated from the previous range to include 2025, which keeps the license information current.
31-31: Good enhancement withstatic constexpr.The change from
const size_ttostatic constexpr size_timproves the implementation by:
- Making the constant truly compile-time
- Ensuring only one instance exists across all class instances
- Following modern C++ best practices
| TEST_F(SortTest, isSortedTest ) { | ||
| int32_t arr[] = {1,2,3,4,5}; | ||
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>); | ||
| EXPECT_TRUE(sorted); | ||
| } |
There was a problem hiding this comment.
Verify comparison function usage in isSortedTest.
The test is using compDescending but expecting a sorted array in ascending order to return true. This seems contradictory and may lead to unexpected behavior.
- bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>);
+ bool sorted = isSorted(arr, 5, sizeof(int32_t), compAscending<int32_t>);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_F(SortTest, isSortedTest ) { | |
| int32_t arr[] = {1,2,3,4,5}; | |
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>); | |
| EXPECT_TRUE(sorted); | |
| } | |
| TEST_F(SortTest, isSortedTest ) { | |
| int32_t arr[] = {1,2,3,4,5}; | |
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compAscending<int32_t>); | |
| EXPECT_TRUE(sorted); | |
| } |
| TEST_F(SortTest, isNotSortedTest) { | ||
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | ||
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>); | ||
| EXPECT_FALSE(sorted); | ||
| } |
There was a problem hiding this comment.
Verify comparison function usage in isNotSortedTest.
Similar to the previous test, this is using compDescending on an array that appears to be in ascending order. Ensure this is the intended behavior.
- bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>);
+ bool sorted = isSorted(arr, 5, sizeof(int32_t), compAscending<int32_t>);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_F(SortTest, isNotSortedTest) { | |
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | |
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compDescending<int32_t>); | |
| EXPECT_FALSE(sorted); | |
| } | |
| TEST_F(SortTest, isNotSortedTest) { | |
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | |
| bool sorted = isSorted(arr, 5, sizeof(int32_t), compAscending<int32_t>); | |
| EXPECT_FALSE(sorted); | |
| } |
| TEST_F(SortTest, binSearchTest) { | ||
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | ||
| quicksort(arr, 5, sizeof(int32_t), compDescending<int32_t>); | ||
| int32_t idx = binSearch(3, arr, 5, compDescending<int32_t>); | ||
| EXPECT_EQ(idx, 2); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix and enhance the binary search test.
The current test has several issues:
- No verification that the search returns -1 for values not in the array
- Uses the same comparison function for sorting and searching
TEST_F(SortTest, binSearchTest) {
int32_t arr[] = { 1, 2, 3, 5, 4 };
- quicksort(arr, 5, sizeof(int32_t), compDescending<int32_t>);
- int32_t idx = binSearch(3, arr, 5, compDescending<int32_t>);
+ quicksort(arr, 5, sizeof(int32_t), compAscending<int32_t>);
+ int32_t idx = binSearch(3, arr, 5, compAscending<int32_t>);
EXPECT_EQ(idx, 2);
+
+ // Test searching for a value not in the array
+ idx = binSearch(6, arr, 5, compAscending<int32_t>);
+ EXPECT_LT(idx, 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_F(SortTest, binSearchTest) { | |
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | |
| quicksort(arr, 5, sizeof(int32_t), compDescending<int32_t>); | |
| int32_t idx = binSearch(3, arr, 5, compDescending<int32_t>); | |
| EXPECT_EQ(idx, 2); | |
| } | |
| TEST_F(SortTest, binSearchTest) { | |
| int32_t arr[] = { 1, 2, 3, 5, 4 }; | |
| quicksort(arr, 5, sizeof(int32_t), compAscending<int32_t>); | |
| int32_t idx = binSearch(3, arr, 5, compAscending<int32_t>); | |
| EXPECT_EQ(idx, 2); | |
| // Test searching for a value not in the array | |
| idx = binSearch(6, arr, 5, compAscending<int32_t>); | |
| EXPECT_LT(idx, 0); | |
| } |
| inline int32_t binSearchImpl(void *key, void *data, size_t num, size_t stride, ComparisonFn func) { | ||
| size_t offset = 0; | ||
| uint8_t *_data = (uint8_t *)data; | ||
| for (size_t i = num; offset < i;) { | ||
| size_t idx = (offset + i) / 2; | ||
| int32_t result = func(key, &_data[i * stride]); | ||
| if (result < 0) { | ||
| i = idx; | ||
| } else if (result > 0) { | ||
| offset = idx + 1; | ||
| } else { | ||
| return idx; | ||
| } | ||
| } | ||
| return ~offset; | ||
| } |
There was a problem hiding this comment.
Fix critical bug in binary search implementation.
There's a serious bug in the binary search implementation: the comparison is done with &_data[i * stride] where i can be as large as num, potentially causing out-of-bounds memory access.
inline int32_t binSearchImpl(void *key, void *data, size_t num, size_t stride, ComparisonFn func) {
size_t offset = 0;
uint8_t *_data = (uint8_t *)data;
for (size_t i = num; offset < i;) {
size_t idx = (offset + i) / 2;
- int32_t result = func(key, &_data[i * stride]);
+ int32_t result = func(key, &_data[idx * stride]);
if (result < 0) {
i = idx;
} else if (result > 0) {
offset = idx + 1;
} else {
return idx;
}
}
return ~offset;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline int32_t binSearchImpl(void *key, void *data, size_t num, size_t stride, ComparisonFn func) { | |
| size_t offset = 0; | |
| uint8_t *_data = (uint8_t *)data; | |
| for (size_t i = num; offset < i;) { | |
| size_t idx = (offset + i) / 2; | |
| int32_t result = func(key, &_data[i * stride]); | |
| if (result < 0) { | |
| i = idx; | |
| } else if (result > 0) { | |
| offset = idx + 1; | |
| } else { | |
| return idx; | |
| } | |
| } | |
| return ~offset; | |
| } | |
| inline int32_t binSearchImpl(void *key, void *data, size_t num, size_t stride, ComparisonFn func) { | |
| size_t offset = 0; | |
| uint8_t *_data = (uint8_t *)data; | |
| for (size_t i = num; offset < i;) { | |
| size_t idx = (offset + i) / 2; | |
| int32_t result = func(key, &_data[idx * stride]); | |
| if (result < 0) { | |
| i = idx; | |
| } else if (result > 0) { | |
| offset = idx + 1; | |
| } else { | |
| return idx; | |
| } | |
| } | |
| return ~offset; | |
| } |
| bool isSorted(const void *data, size_t num, size_t stride, ComparisonFn func) { | ||
| if (num < 2) { | ||
| return true; | ||
| } | ||
| uint8_t *data_ = (uint8_t *)data; | ||
| for (size_t i=1; i<num; ++i) { | ||
| const int32_t result = func(&data_[(i-1)*stride], &data_[i * stride]); | ||
| if (result == -1) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the isSorted check more robust.
The function checks if result == -1 to determine if an array is not sorted, but this assumes the comparison function returns specific values. It would be more robust to check if result < 0.
bool isSorted(const void *data, size_t num, size_t stride, ComparisonFn func) {
if (num < 2) {
return true;
}
uint8_t *data_ = (uint8_t *)data;
for (size_t i=1; i<num; ++i) {
const int32_t result = func(&data_[(i-1)*stride], &data_[i * stride]);
- if (result == -1) {
+ if (result < 0) {
return false;
}
}
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool isSorted(const void *data, size_t num, size_t stride, ComparisonFn func) { | |
| if (num < 2) { | |
| return true; | |
| } | |
| uint8_t *data_ = (uint8_t *)data; | |
| for (size_t i=1; i<num; ++i) { | |
| const int32_t result = func(&data_[(i-1)*stride], &data_[i * stride]); | |
| if (result == -1) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| bool isSorted(const void *data, size_t num, size_t stride, ComparisonFn func) { | |
| if (num < 2) { | |
| return true; | |
| } | |
| uint8_t *data_ = (uint8_t *)data; | |
| for (size_t i=1; i<num; ++i) { | |
| const int32_t result = func(&data_[(i-1)*stride], &data_[i * stride]); | |
| if (result < 0) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } |
|
Summary by CodeRabbit