Fix binsearch #36
Fix binsearch #36
Conversation
WalkthroughThis pull request extends functionalities in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant QS as quicksort
participant QI as quicksortImpl
participant Swap as swap functions
participant CF as comparison functions
Caller->>QS: Call quicksort(data, num, stride, comp)
QS->>QI: Delegate to quicksortImpl()
QI->>CF: Compare elements during partition
CF-->>QI: Return comparison result
QI->>Swap: Perform swap operations as needed
Swap-->>QI: Swapped elements
QI-->>QS: Return sorted data
QS-->>Caller: Provide sorted array
sequenceDiagram
participant User as User
participant BS as binSearch
participant BI as binSearchImpl
participant CF as comparison function
User->>BS: Call binSearch(key, data, num, comp)
BS->>BI: Forward call to binSearchImpl()
BI->>CF: Compare key with current element
CF-->>BI: Return comparison outcome
BI-->>BS: Return index (or -1 if not found)
BS-->>User: Provide search result
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: 0
🔭 Outside diff range comments (1)
include/cppcore/Common/Sort.h (1)
140-153: 🛠️ Refactor suggestionImprove the isSorted function to handle different ordering.
The current implementation of
isSortedonly returns false whenresult == -1, but doesn't handle cases whereresult > 0. This implies that the function only works correctly for ascending order. Consider improving the implementation to make it more flexible: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; }This change would properly check for array ordering according to the provided comparison function, regardless of whether it's for ascending or descending order.
🧹 Nitpick comments (5)
include/cppcore/Memory/TPoolAllocator.h (1)
61-62: Fix the typo in the documentation.There's a typo in the documentation: "WIll" should be "Will" for consistency and professionalism.
- /// @brief WIll alloc one item from the pool. + /// @brief Will alloc one item from the pool.include/cppcore/Common/Sort.h (4)
49-52: Maintain consistent template parameter naming.The template parameter naming is inconsistent between
compAscending(usesT) andcompDescending(usesTy). For better code consistency, use the same template parameter name in both functions.-template <typename Ty> -inline int32_t compDescending(const void *_lhs, const void *_rhs) { - return compAscending<Ty>(_rhs, _lhs); +template <typename T> +inline int32_t compDescending(const void *_lhs, const void *_rhs) { + return compAscending<T>(_rhs, _lhs); }
77-84: Use consistent parameter naming in function signature and body.The parameter names in the function signature (
v1,v2) don't match the names used in the function body (lhs,rhs), which can cause confusion for readers. Consider unifying the naming convention:-inline void swap(void *v1, void *v2, size_t stride) { - uint8_t *lhs = (uint8_t*) v1; - uint8_t *rhs = (uint8_t*) v2; +inline void swap(void *lhs, void *rhs, size_t stride) { + uint8_t *lhs_ptr = (uint8_t*) lhs; + uint8_t *rhs_ptr = (uint8_t*) rhs; - const uint8_t *end = rhs + stride; + const uint8_t *end = rhs_ptr + stride; - while (rhs != end) { - swap(*lhs++, *rhs++); + while (rhs_ptr != end) { + swap(*lhs_ptr++, *rhs_ptr++); } }
162-177: Improve binary search implementation and documentation.The current return value documentation could be clearer about what the negative return value means. Also, there's an opportunity to improve the loop condition's readability:
/// @brief Implements a binary search algorithm. /// @param key The key to search for /// @param data The data to search in /// @param num The number of elements to search /// @param stride The stride of the data, i.e. the size of each element /// @param func The comparison function -/// @return The index of the key if found, otherwise ~index +/// @return The index of the key if found, otherwise the bitwise complement (~) of the insertion point inline int32_t binSearchImpl(const void *key, const void *data, size_t num, size_t stride, ComparisonFn func) { size_t offset = 0; const uint8_t *_data = (uint8_t *)data; - for (size_t i = num; offset < i;) { + for (size_t high = num; offset < high;) { - size_t idx = (offset + i) / 2; + size_t idx = (offset + high) / 2; int32_t result = func(key, &_data[idx * stride]); if (result < 0) { - i = idx; + high = idx; } else if (result > 0) { offset = idx + 1; } else { return idx; } } return ~offset; }This change improves both the documentation and code readability by making it clearer that
highrepresents the upper bound of the search space.
92-122: Consider further optimizing the quicksort implementation.The quicksort implementation is functional, but there are a few potential optimizations to consider:
- For very small arrays (e.g., less than 10 elements), insertion sort is often faster than quicksort due to its lower overhead.
- The current implementation uses the first element as the pivot, which can lead to worst-case O(n²) performance on already sorted or reverse-sorted arrays.
Consider incorporating these optimizations:
inline void quicksortImpl(void *pivot, void *_data, size_t num, size_t stride, ComparisonFn func) { if (num < 2) { return; } if (_data == nullptr) { return; } + // Use insertion sort for small arrays + if (num < 10) { + uint8_t *data = (uint8_t*) _data; + for (size_t i = 1; i < num; ++i) { + for (size_t j = i; j > 0; --j) { + if (func(&data[(j-1)*stride], &data[j*stride]) <= 0) { + break; + } + swap(&data[(j-1)*stride], &data[j*stride], stride); + } + } + return; + } uint8_t *data = (uint8_t*) _data; - memcpy(pivot, &data[0], stride); + // Choose a better pivot (median of three) + size_t mid = num / 2; + size_t last = num - 1; + if (func(&data[0], &data[mid*stride]) > 0) { + swap(&data[0], &data[mid*stride], stride); + } + if (func(&data[0], &data[last*stride]) > 0) { + swap(&data[0], &data[last*stride], stride); + } + if (func(&data[mid*stride], &data[last*stride]) > 0) { + swap(&data[mid*stride], &data[last*stride], stride); + } + memcpy(pivot, &data[mid*stride], stride); // Rest of the implementation...These changes would make the quicksort algorithm more efficient across different input distributions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
include/cppcore/Common/Sort.h(5 hunks)include/cppcore/Memory/TPoolAllocator.h(1 hunks)
Summary by CodeRabbit
New Features
Documentation