Weight function implementations unnecessarily verbose

[benluelo]

Several of the Weight implementations are (as the title states) unnecessarily verbose:

• checked_* functions could be made much more succinct by using the ? operator not possible, see Tracking Issue for the use of ? in constants rust-lang/rust#74935

• the any_*/all_* functions all follow the same pattern:

  // all_*
  self.ref_time OP other.ref_time && self.proof_size OP other.proof_size
  
  // any_*
  self.ref_time OP other.ref_time || self.proof_size OP other.proof_size
  

  which could very easily be implemented with a macro_rules!, and would reduce maintenance burden of any fields added in the future.

• the scalar arithmetic ops have their implementation duplicated, for example mul here:

  • https://github.com/paritytech/substrate/blob/master/primitives/weights/src/weight_v2.rs#L274-L276
  • https://github.com/paritytech/substrate/blob/master/primitives/weights/src/weight_v2.rs#L365-L373

  unfortunately not possible due to the current trait bounds on <Weight as Mul<T>>::mul

Also, Weight doesn't impl CheckedMul, which I think it should. not possible either

As always, happy to pick this up if it's accepted 🙂

[bkchr]

• which could very easily be implemented with a macro_rules!, and would reduce maintenance burden of any fields added in the future.

I'm not convinced by this, because this isn't changing that often.

the scalar arithmetic ops have their implementation duplicated, for example mul here:

* https://github.com/paritytech/substrate/blob/master/primitives/weights/src/weight_v2.rs#L274-L276

* https://github.com/paritytech/substrate/blob/master/primitives/weights/src/weight_v2.rs#L365-L373

Fine point, the trait could call the const method.

Also, Weight doesn't impl CheckedMul, which I think it should.

Also okay.

[benluelo]

For the first point, I was thinking that it would be nice to define those through a sort of "pseudo-function-composition", and cut down on the boilerplate a bit (the maintenance cost was just a nice bonus), like this: https://gist.github.com/benluelo/07d58471716ef211cd359b915bea4021.

Some unfortunate issues:

• For CheckedMul, Weight doesn't impl Mul<Weight> (which makes sense - I originally read the scalar impls incorrectly, my bad there). My need for this stemmed from Defensive ops API design polkadot-sdk#221, since I couldn't use DefensiveSaturating::defensive_saturating_add with Weight. num_traits::CheckedMul expects Mul<Self, Output = Self> (annoyingly), so a scalar CheckedMul impl isn't possible anyways.

• <Weight as Mul>::mul can't call into const Weight::mul since since that fn expects a u64.

• ? isn't stable in const contexts yet (I tend to have this feature enabled in my personal projects, I forgot it wasn't stable :( ), so something like this could be done: https://gist.github.com/benluelo/aa63c166398f5f74b123081ed7da7e58

Also, for add and sub, what are your thoughts on separating the scalar methods' naming? As of right now, the non-scalar add and sub fns aren't const (as they're trait methods), and said const impls can't be added without choosing a different name for them. It's also slightly confusing to have two add methods that work differently.

[ggwpez]

For the first point, I was thinking that it would be nice to define those through a sort of "pseudo-function-composition", and cut down on the boilerplate a bit (the maintenance cost was just a nice bonus)

About the syntax: We actually want less syntax magic and more data driven so that downstream tooling can easier parse it: paritytech/polkadot-sdk#382
Something similar to the WeightToFeeCoefficients maybe.
Going to re-read your issue tomorrow and answer the other points 😄