IncDec: add in-place inc_mut/dec_mut and collapse impls into recursive blankets#5
Conversation
EgorKulikov
left a comment
There was a problem hiding this comment.
Thanks for this — the in-place-first / recursive direction is nice, and the inc_mut/dec_mut + provided inc/dec split is clean. Two blocking issues need to be resolved before this can merge, both stemming from the recursive tuple impls changing behavior for the shapes this trait actually exists to serve (competitive I/O, where tuples are (coord, coord, payload)).
1. (T, U, V) now shifts the third field — silent regression, fails an existing test
The old Vec<(T, U, V)> impl incremented only fields .0 and .1 (|(i, j, _)|), deliberately leaving the third field untouched — that slot is a payload/weight (edge capacity, cost, char, …), and it had no IncDec/numeric bound at all. The new generic (T, U, V) impl increments all three fields and requires V: IncDec.
This is a real regression, not hypothetical — it breaks an in-tree test:
$ cargo test -p algo_lib --test download_speed
...
Expected: 6
Output: 4
Verdict: Wrong Answer (Token #0 differs, expected 6, actual 4)
download_speed.rs reads read_vec::<(usize, usize, i64)>(m).dec() where the third field is edge capacity; it now gets decremented, corrupting the max-flow input. CI (cargo test) will go red. It also tightens bounds: (usize, usize, char) payloads no longer compile (char: !IncDec).
Please preserve the "first two fields are coordinates, rest is untouched payload" semantics for tuple records (dedicated coordinate-only tuple impls with no bound on the payload types), or — if all-fields recursion is genuinely intended — audit and fix every (u, v, weight).dec() call site and document the change loudly.
2. Vec<(T, U, V, W)> and Vec<(T, U, V, W, X)> support dropped with no replacement
The 4- and 5-tuple Vec impls were removed and there is no generic 4-/5-tuple impl, so Vec<(T,U,V,W)>.dec() / Vec<(T,U,V,W,X)>.dec() no longer compile. These don't "compose automatically" from the 2-/3-tuple impls. There's no current workspace caller (so cargo test won't flag it), but this is a public-API contraction that breaks downstream/contest code. Please restore 4-/5-tuple coverage — which falls out naturally if you keep the coordinate-only tuple semantics from (1).
Both issues are resolved cleanly by keeping dedicated coordinate-only tuple impls (touch .0/.1, leave the rest unbounded and untouched) rather than recursing into every field.
EgorKulikov
left a comment
There was a problem hiding this comment.
Verified locally: full test suite green (196 suites, 0 failures), coordinate-only tuple semantics preserved for arities 2–5 with unbounded payloads, and the in-place path works. Thanks for the quick turnaround!
|
Thanks for quick reply! |
Reworks the
IncDectrait to be in-place-first and recursive, replacing the hand-written per-shape implementations with a small set of composable blanket impls.inc_mut(&mut self)/dec_mut(&mut self)as the trait's core operations. The existinginc(self)/dec(self)become provided methods implemented on top of them, so callers keep the same by-value API while in-place mutation is now available for free.inc_mut/dec_mutbodies.Vec<T>,Vec<Vec<T>>,Vec<(T, U)>,Vec<(T, U, V)>,Vec<(T, U, V, W)>,Vec<(T, U, V, W, X)>,(T, U)) into:impl<T: IncDec> IncDec for Vec<T>, andimpl IncDec for (U, V)and(T, U, V).Because the impls are now recursive over
IncDecrather thanAdditionMonoidWithSub + One, nested shapes compose automatically:Vec<Vec<T>>,Vec<(T, U)>,Vec<(T, U, V)>, etc. all work without dedicated impls.