Skip to content

Conversation

quaternic
Copy link
Contributor

@quaternic quaternic commented Aug 12, 2025

Builds on #1011, both were split from #1002

The computation is implemented in linear_mul_reduction.

Comment on lines 301 to 304
+ core::ops::Add<Output = Self>
+ core::ops::Sub<Output = Self>
+ core::ops::Shl<u32, Output = Self>
+ core::ops::Shr<u32, Output = Self>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core::ops -> ops since it's already in scope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit should be able to allow the #[allow(dead_code)] on NarrowingDiv

Comment on lines +1 to +9
use crate::support::int_traits::NarrowingDiv;
use crate::support::{DInt, HInt, Int};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a module-level doc comment with some of the common names used here? E.g. R, RR (if that's not R*R) r, m, q, xq. I'm unfortunately a bit lost :) (but I don't need to understand it in detail)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've authored this, add /* SPDX-License-Identifier: MIT OR Apache-2.0 */ as well (or only MIT if it's derived, as appropriate)

}

#[cfg(test)]
mod test {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this spot check another integer size as well? Just using constants

@quaternic
Copy link
Contributor Author

I've now rebased and applied the previous feedback.

@tgross35

Could you add a module-level doc comment with some of the common names used here? E.g. R, RR (if that's not R*R) r, m, q, xq. I'm unfortunately a bit lost :) (but I don't need to understand it in detail)

To make the module easier to follow, I moved the public interface, fn linear_mul_reduction, to the start of the file and added more explanatory comments. That should be understandable on its own. The implementation is essentially unchanged from before, other than one minor tweak.

Comment on lines +54 to +57
// The partial remainder is in `[0, 2y)` ...
let r = m.partial_remainder();
// ... so check and correct, and compensate for the earlier shift.
r.checked_sub(y).unwrap_or(r) >> s
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That minor tweak:
Previously the right shift was done first, and then the checked_sub was with the original y. Now that's done first with the left-shifted y, and the shift is last.

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.

2 participants