-
Notifications
You must be signed in to change notification settings - Fork 601
Add DivRemGeneric trait #7722
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add DivRemGeneric trait #7722
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion
corelib/src/traits.cairo
line 367 at r1 (raw file):
} /// Performs truncated division **and** remainder.
new traits are not added here - adding traits here is deprecated.
this should probably go to num/traits/ops
and should probably additionally lose the Generic
in the name.
Thanks for the feedback! Will move to 'num/traits/ops'. For future reference, where could I have found that adding traits in 'corelib/src/traits' was deprecated? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was just an internal decision - you can just see all new traits were added elsewhere.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @0xpantera)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r2, all commit messages.
Reviewable status: 1 of 11 files reviewed, 3 unresolved discussions (waiting on @0xpantera)
corelib/src/traits.cairo
line 367 at r2 (raw file):
} /// Legacy trait for division with remainder.
revert the doc changes - this trait is by no means deprecated.
we must make sure the change fully work before such a thing.
corelib/src/num/traits/ops/divrem.cairo
line 74 at r2 (raw file):
pub impl BridgeU256 = Bridge<u256>; } use _divrem_bridge::*;
- starting with
_
means unused - this is just private. - Either impl for all - and therefore no inner module is required, or impl specifically - and then the impl aliases should be out of the module - the
use *
did both.
Suggestion:
mod by_divrem_legacy {
/// Generic adapter: if the old symmetric `DivRem<T>` exists,
/// provide the corresponding `DivRemGeneric<T,T>` implementation.
impl Impl<T, +crate::traits::DivRem<T>> of super::DivRem<T, T> {
type Quotient = T;
type Remainder = T;
fn div_rem(lhs: T, rhs: NonZero<T>) -> (T, T) {
crate::traits::DivRem::<T>::div_rem(lhs, rhs)
}
}
}
// Instantiate the generic adapter for every concrete integer type
// that already has a symmetric `DivRem` implementation.
pub impl DivRemU8 = by_divrem_legacy::Impl<u8>;
pub impl BridgeU16 = by_divrem_legacy::Impl<u16>;
pub impl BridgeU32 = by_divrem_legacy::Impl<u32>;
pub impl BridgeU64 = by_divrem_legacy::Impl<u64>;
pub impl BridgeU128 = by_divrem_legacy::Impl<u128>;
pub impl BridgeU256 = by_divrem_legacy::Impl<u256>;
0e81f17
to
960bacc
Compare
* New file `corelib/src/num/traits/ops/divrem.cairo` – defines the generic `DivRem<T,U>` trait (was in core traits.cairo). – adds the `by_divrem_legacy::Bridge` adapter that maps every existing symmetric `crate::traits::DivRem<T>` implementation to `DivRem<T,T>` so old code keeps compiling. – publishes concrete aliases (`DivRemU8 … DivRemU256`) outside the module, per review guidelines. * `corelib/src/num/traits/ops.cairo` – re-exports `DivRem`, making it part of the public numeric-ops API. * `corelib/src/num/traits.cairo` – bubbles the new trait up alongside the other `num::traits` re-exports. * `corelib/src/traits.cairo` – removes the now-redundant generic-DivRem definition and restores the legacy `DivRem<T>` docs (no longer marked deprecated). These edits address reviewer feedback on PR starkware-libs#7722: • trait must live under `num/traits/ops` • drop the “Generic” suffix • inner adapter module should stay private; concrete `pub impl`s live at top level. All existing tests pass, and the new `divrem` tests in `num_test.cairo` verify both generic and legacy paths.
@orizi Appreciate the feedback. I have reverted the doc changes and fixed the legacy bridge implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 25 files reviewed, 4 unresolved discussions (waiting on @0xpantera)
-- commits
line 3 at r3:
fix your rebase - it now contains random stuff.
* New file `corelib/src/num/traits/ops/divrem.cairo` – defines the generic `DivRem<T,U>` trait (was in core traits.cairo). – adds the `by_divrem_legacy::Bridge` adapter that maps every existing symmetric `crate::traits::DivRem<T>` implementation to `DivRem<T,T>` so old code keeps compiling. – publishes concrete aliases (`DivRemU8 … DivRemU256`) outside the module, per review guidelines. * `corelib/src/num/traits/ops.cairo` – re-exports `DivRem`, making it part of the public numeric-ops API. * `corelib/src/num/traits.cairo` – bubbles the new trait up alongside the other `num::traits` re-exports. * `corelib/src/traits.cairo` – removes the now-redundant generic-DivRem definition and restores the legacy `DivRem<T>` docs (no longer marked deprecated). These edits address reviewer feedback on PR starkware-libs#7722: • trait must live under `num/traits/ops` • drop the “Generic” suffix • inner adapter module should stay private; concrete `pub impl`s live at top level. All existing tests pass, and the new `divrem` tests in `num_test.cairo` verify both generic and legacy paths.
960bacc
to
a0292ef
Compare
Rebased onto current main, resolved the lowering conflicts by keeping |
Add
DivRemGeneric<T, U>
(heterogeneous div-mod) & keepDivRem<T>
workingThis PR implements the generic, asymmetric division-with-remainder trait that had
been marked TODO in
core::traits
.It also supplies a zero-cost compatibility bridge so all existing code that uses the
legacy
DivRem<T>
continues to compile unchanged.Why introduce
DivRemGeneric
?DivRem<T>
(before)DivRemGeneric<T, U>
(now)T / T
)T / U
)(T, T)
(Quotient, Remainder)
u256 / u128
Benefits over legacy
DivRem<T>
Mixed-width math without ad-hoc helpers
DivRemGeneric<u256, u128>
lets us expressu256 / u128
directly, instead of exposingu256_safe_div_rem
and friends.Accurate associated types
Call-sites get the exact
Quotient
/Remainder
types from the trait, enablinggeneric algorithms (e.g.
wide_div_round
) to compile for any(T,U)
pair.No breaking changes today
The bridge keeps
DivRem<T>
working, so downstream crates can migrate at theirown pace while getting deprecation nudges.
What’s in the patch?
corelib/src/traits.cairo
trait DivRemGeneric<T, U>
•
#[deprecated] trait DivRem<T>
(unchanged API)• Internal module
_divrem_bridge
that automatically providesDivRemGeneric<T,T>
for every pre-existingDivRem<T>
impl (u8, u16, u32, u64, u128, u256).corelib/src/integer.cairo
corelib/src/test/divrem_generic_test.cairo
• legacy symmetric path (
u32 / u32
)• generic asymmetric path (
u256 / u128
).corelib/src/lib.cairo
#[feature("generic-divrem")]
gate added.Migration / Back-compat
Code that calls
DivRem::<T>::div_rem
compiles exactly as before (now routedthrough the bridge).
core::traits::DivRemGeneric
.DivRem<T>
is soft-deprecated with the attribute:The warning only appears when a crate doesn’t enable the new feature flag,
so corelib itself builds cleanly.