Add new traits for reference and assignment operators #283
Conversation
traits/src/lib.rs
Outdated
| /// second operand either by value or by reference. | ||
| /// | ||
| /// This is automatically implemented for all `&T` implementing the operators. | ||
| pub trait RefNum<Base>: NumOps<Base, Base> + for<'r> NumOps<&'r Base, Base> {} |
There was a problem hiding this comment.
I don't like taking the Base type as a parameter, but I didn't find a better way to do this. I tried adding a Deref constraint, but the compiler didn't seem to see that <Self as Deref>::Target always matches the types we want, especially when chaining as in the check_refnum_ops test.
traits/src/lib.rs
Outdated
| /// | ||
| /// This is automatically implemented for all `&T` implementing the operators. | ||
| pub trait RefNum<Base>: NumOps<Base, Base> + for<'r> NumOps<&'r Base, Base> {} | ||
| impl<'a, T> RefNum<T> for &'a T where &'a T: NumOps<T, T> + for<'r> NumOps<&'r T, T> {} |
There was a problem hiding this comment.
Why limit this impl to &T, rather than everything satisfying the trait constraints?
impl<Base, T> RefNum<Base> for T where T: NumOps<Base, Base> + for<'r> NumOps<&'r Base, Base> {}There was a problem hiding this comment.
In part, because I was hoping to find a way not to need the Base parameter at all. I'm stuck on that though.
I also think it's a little weird that Base = T would be allowed, as I'm trying to specify a reference type here. I was going for NumRef for values with numeric operators, and RefNum for references with numeric operators. Maybe that distinction isn't so useful? I'm not sure...
I probably could narrow it with RefNum<Base>: Deref<Target = Base> + ...
There was a problem hiding this comment.
The more I think about it, the more I like adding Deref in there, to lock down the intended relationship between T and Base. If I can't get rid of Base, at least I can make it have only one possible answer -- Base = <T as Deref>::Target.
There was a problem hiding this comment.
I can’t seem to make it work without the Base parameter either. This may be an inference bug. But unless Base can actually be avoided, such narrowing seems to add nothing to the present use case—in for<'a> &'a T: RefNum<T>, we don’t need to tell the compiler again that &'a T is a reference—and it might exclude other use cases we haven’t thought of.
There was a problem hiding this comment.
I'm trying to think if there are hypothetical situations where you'd want a clear distinction between RefNum and NumRef. Maybe for specialization? But I think you could use private traits with a Deref or similar differentiator. Dunno.
I'll play a little more, and as long I can't find any problem doing it your way, I'll switch it.
Thanks for your input!
| + Mul<Rhs, Output = Output> | ||
| + Div<Rhs, Output = Output> | ||
| + Rem<Rhs, Output = Output> | ||
| {} |
There was a problem hiding this comment.
Can Output be made into an associated type here? (I’m having trouble getting that to work, though.)
There was a problem hiding this comment.
I can't find a way to specify that an associated Output and all the individual ops' Output must be identical. If you figure it out, let me know...
|
OK, I switched to a more generic |
|
I'll merge this in a day or two if there are no further comments... |
|
@homu r+ |
|
📌 Commit 68028d5 has been approved by |
|
🔒 Merge conflict |
|
☔ The latest upstream changes (presumably #286) made this pull request unmergeable. Please resolve the merge conflicts. |
There are two new "utility" traits covering the basic operators: `Add`, `Sub`, `Mul`, `Div`, and `Rem`. - `NumOps<Rhs, Output>`: operators with an arbitrary operand and output. - `NumAssignOps<Rhs>`: assignment operators with an arbitrary operand. Then the new collection of numeric traits are: - `Num`: effectively unchanged, just taking operands by value. - `NumRef`: `Num` adding reference operands on the right side. - `RefNum`: `&T` operators, with either `T` or `&T` on the right side. - This does not specify `T: Num`, as rust-lang/rust#20671 means that could only add a constraint, without implying its presence for use. - `NumAssign`: `Num` adding assignment operators by value. - `NumAssignRef`: `NumAssign` adding reference assignment operators. - Nothing actually implements this yet! Acknowledgement: this is roughly based on [@andersk's suggestion][1]. [1] rust-num#94 (comment)
|
@homu r+ |
|
📌 Commit 3ead4a1 has been approved by |
Add new traits for reference and assignment operators There are two new "utility" traits covering the basic operators: `Add`, `Sub`, `Mul`, `Div`, and `Rem`. - `NumOps<Rhs, Output>`: operators with an arbitrary operand and output. - `NumAssignOps<Rhs>`: assignment operators with an arbitrary operand. Then the new collection of numeric traits are: - `Num`: effectively unchanged, just taking operands by value. - `NumRef`: `Num` adding reference operands on the right side. - `RefNum`: `&T` operators, with either `T` or `&T` on the right side. - This does not specify `T: Num`, as rust-lang/rust#20671 means that could only add a constraint, without implying its presence for use. - `NumAssign`: `Num` adding assignment operators by value. - `NumAssignRef`: `NumAssign` adding reference assignment operators. - Nothing actually implements this yet! Acknowledgement: this is roughly based on [@andersk's suggestion](#94 (comment)).
|
☀️ Test successful - status |
There are two new "utility" traits covering the basic operators:
Add,Sub,Mul,Div, andRem.NumOps<Rhs, Output>: operators with an arbitrary operand and output.NumAssignOps<Rhs>: assignment operators with an arbitrary operand.Then the new collection of numeric traits are:
Num: effectively unchanged, just taking operands by value.NumRef:Numadding reference operands on the right side.RefNum:&Toperators, with eitherTor&Ton the right side.T: Num, as where clauses are only elaborated for supertraits, and not other things rust-lang/rust#20671 means thatcould only add a constraint, without implying its presence for use.
NumAssign:Numadding assignment operators by value.NumAssignRef:NumAssignadding reference assignment operators.Acknowledgement: this is roughly based on @andersk's suggestion.