fix(iter): make combinations and permutations generic #223
Conversation
WalkthroughThe pull request introduces generic type support to the combination and permutation iterator functions in the Changes
Sequence DiagramsequenceDiagram
participant User
participant CombinationsFunc
participant CombinationsIter
User->>CombinationsFunc: Call with generic type array
CombinationsFunc->>CombinationsIter: Create iterator
CombinationsIter-->>User: Return combinations
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (2)
iter/perm_test.v (1)
75-80: Consider adding tests with duplicate elements.
While these generic type tests show coverage for distinct elements, including tests with duplicate elements could further validate correctness.iter/comb_test.v (1)
47-53: Typographical consistency: rename "genric" to "generic."
Small improvement suggestion for clarity.-fn test_combinations_with_genric_type_string() { +fn test_combinations_with_generic_type_string() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
iter/comb.v(4 hunks)iter/comb_test.v(1 hunks)iter/perm.v(3 hunks)iter/perm_test.v(1 hunks)
🔇 Additional comments (9)
iter/perm.v (2)
6-14: Great improvement for supporting generic types.
The introduction of a type parameter [T] in the struct and its data field expands usability beyond just floating-point arrays.
Line range hint 40-72: Efficient iteration logic.
The logic in the nextT method efficiently generates permutations. The approach is clear and well-structured, properly returning none when iteration completes.
iter/comb.v (5)
7-9: Generic combinations function is well-designed.
Switching to generics is seamless, and the pre-allocation with cap: int(combinations.size) is a good performance optimization.
16-23: Data field now correctly matches the generic type.
The field data []T aligns well with the rest of the struct’s generic definition, ensuring consistency across the module.
83-85: Consistency with function naming.
The new combinations_with_replacementT function matches the style used for permutations. Code remains clean and consistent.
92-114: Struct and constructor updates maintain a predictable pattern.
Good job mirroring the changes from CombinationsIter to CombinationsWithReplacementIter, ensuring tests remain consistent.
123-123: Well-structured nextT method.
The approach handles repetition elegantly and returns none once exhausted, matching the combinatorial logic.
iter/perm_test.v (1)
67-73: Additional tests for strings.
Testing permutations with string data ensures the new generic logic is validated across different types.
iter/comb_test.v (1)
54-60: Good coverage for integer combinations.
The addition of integer dataset testing confirms that combinations work for non-floating types.
| pub fn PermutationsIter.new[T](data []T, r int) PermutationsIter[T] { | ||
| n := data.len | ||
| if r > n { | ||
| return PermutationsIter{ | ||
| return PermutationsIter[T]{ | ||
| data: data | ||
| repeat: r | ||
| } | ||
| } | ||
| size := u64(math.factorial(n) / math.factorial(n - r)) | ||
| idxs := util.arange(n) | ||
| cycles := util.range(n, n - r, step: -1) | ||
| return PermutationsIter{ | ||
| return PermutationsIter[T]{ |
There was a problem hiding this comment.
💡 Codebase verification
Potential integer overflow risk requires bounds checking
Based on the code inspection, the PermutationsIter.new function uses factorial calculations for both n and n-r without any bounds checking. While we couldn't locate the exact implementation of math.factorial, the mathematical nature of factorials means they grow extremely rapidly, potentially causing integer overflow. The risk is particularly high since:
- The function accepts arbitrary length arrays (
data []T) - There's only a basic check for
r > n - The factorial calculation
math.factorial(n) / math.factorial(n - r)could overflow even before the division
Recommendations:
- Add upper bounds validation for
n(e.g., n ≤ 20 for 64-bit integers) - Consider using a checked arithmetic approach or BigInt for larger values
- Add error handling for potential overflow cases
🔗 Analysis chain
Potential factorial overflow for large n.
When n is large, factorial(n) can become huge very quickly. Consider validating n or using a checked approach to prevent overflow or performance issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking all usage of PermutationsIter.new to verify the range of r and data length
rg --multiline "PermutationsIter\.new\s*\(\s*\w+,\s*\w+\)" -A 5
Length of output: 65
Script:
#!/bin/bash
# Let's try a different approach to find usages of PermutationsIter
rg "PermutationsIter" -A 5
# Also check the implementation of math.factorial
ast-grep --pattern 'fn factorial($_) {
$$$
}'
Length of output: 1314
Script:
#!/bin/bash
# Let's check the math module for factorial implementation
rg "fn factorial" -A 10
# Also check for any tests that might show usage patterns
fd "test.*\.v" --exec rg "PermutationsIter" -A 5 {}
Length of output: 79
| pub fn CombinationsIter.new[T](data []T, r int) CombinationsIter[T] { | ||
| n := data.len | ||
| if r > n { | ||
| return CombinationsIter{ | ||
| return CombinationsIter[T]{ | ||
| data: data | ||
| repeat: r | ||
| } | ||
| } | ||
| size := u64(fun.choose(n, r)) | ||
| idxs := util.arange(r) | ||
| return CombinationsIter{ | ||
| return CombinationsIter[T]{ |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code found, I can now generate the final response as we have enough information about the implementation of choose and its potential overflow behavior.
Add overflow checks for combinatorial calculations
The current implementation of choose(n, r) uses uint_binomial which has overflow protection, but the size check r > n should happen before any calculations to prevent unnecessary work and potential integer overflow. Consider:
if r > n {
return CombinationsIter[T]{
data: data
repeat: 0 // or handle this case differently
}
}Reasoning:
- The code shows that
uint_binomialhas overflow checks and will panic on overflow - However, the current implementation first checks
r > nafter callingfun.choose(n, r), which could lead to unnecessary calculations - The
choosefunction internally uses factorial calculations which can overflow for large numbers - Moving the size check before any calculations would be more efficient and safer
🔗 Analysis chain
Ensure safe usage of combinatorial expansions.
Similar to permutations, calling fun.choose(n, r) could be large. Check for potential overflow or extremely large memory allocations.
Let me gather information about the implementation of fun.choose and related functions to verify the safety measures.
Let me try to find the implementation of fun.choose specifically.
Let me try a different approach to find the implementation using ripgrep.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find the implementation of fun.choose
ast-grep --pattern 'fn choose($_, $_) {
$$$
}'
# Search for any overflow checks in the codebase
rg -A 5 "overflow"
# Look for the implementation details in fun module
fd "fun\.v$" --exec cat {}
Length of output: 70495
Script:
#!/bin/bash
# Search for choose function implementation
ast-grep --pattern 'fn choose($_, $_) {
$$$
}'
# Also look for binomial coefficient implementation
ast-grep --pattern 'fn uint_binomial($_, $_) {
$$$
}'
Length of output: 108
Script:
#!/bin/bash
# Search for choose function implementation
rg -A 10 "fn choose"
# Also look for binomial coefficient implementation
rg -A 10 "fn uint_binomial"
# Look for any overflow checks related to combinations
rg -A 5 "overflow.*binomial|binomial.*overflow"
Length of output: 2363
This PR makes iter functions like combinations and permutations generic.
I also added test cases for them.
Summary by CodeRabbit
New Features
Bug Fixes
Tests