Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the prime_factorization library to support prime factorization via two strategies (trial division and least-prime-factor precomputation), and adjusts the least_prime_factors crate API to compute factors up to and including n.
Changes:
- Replaced the old “method on integers” API with a strategy-based
PrimeFactorization<T>trait plusTrialDivisionandByLeastPrimeFactorsimplementations. - Added a dependency from
prime_factorizationto the localleast_prime_factorscrate. - Updated
least_prime_factors(n)to return a table covering indices0..=n(inclusive), with updated docs/tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| libs/prime_factorization/src/lib.rs | Introduces strategy-based factorization APIs and adds LPF-backed implementation + tests/docs. |
| libs/prime_factorization/Cargo.toml | Adds a path dependency on least_prime_factors. |
| libs/least_prime_factors/src/lib.rs | Changes LPF sieve to compute results for 2..=n and updates documentation/tests accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// ``` | ||
| pub fn least_prime_factors(n: usize) -> Vec<usize> { | ||
| let mut result = vec![0; n]; | ||
| let mut result = vec![0; n + 1]; |
There was a problem hiding this comment.
vec![0; n + 1] can overflow when n == usize::MAX (wrap in release, panic in debug), leading to confusing behavior/panics later. Consider using n.checked_add(1) (and an explicit panic message or early return) to make the failure mode well-defined.
| let mut result = vec![0; n + 1]; | |
| let size = n.checked_add(1).expect("n is too large to compute least_prime_factors"); | |
| let mut result = vec![0; size]; |
| /// 素因数分解の前計算として [least prime factors](least_prime_factors::least_prime_factors) を求めます。 | ||
| pub fn new(n: usize) -> Self { | ||
| let lpf = least_prime_factors(n); | ||
| Self { lpf } | ||
| } |
There was a problem hiding this comment.
ByLeastPrimeFactors::new(n) creates a table that only supports factors(x) for x <= n (otherwise it panics). The doc comment for new should explicitly state this precondition (and ideally mention the O(n) memory cost) so callers know what range is supported.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // p * p > x | ||
| if p > x / p { |
There was a problem hiding this comment.
In TrialDivision::factors, the loop termination condition uses the original x (if p > x / p) even after y has been reduced by dividing out factors. For inputs like large powers of 2, y quickly becomes 1 but the loop still runs up to sqrt(x), which can be billions of iterations for u64. Use the current remainder (y) in the p*p > ... check (and/or break early when y == 1) so the loop stops once p*p > y.
| // p * p > x | |
| if p > x / p { | |
| // p * p > y | |
| if p > y / p { |
1 participant
No description provided.