implement Add and Sub for Complex - #161227
Conversation
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| impl<T: Add<Output = T>> Add<Self> for Complex<T> { | ||
| type Output = Complex<T::Output>; | ||
|
|
||
| fn add(self, rhs: Self) -> Self::Output { | ||
| Self::new(self.re + rhs.re, self.im + rhs.im) | ||
| } | ||
| } |
There was a problem hiding this comment.
Any reason to limit the RHS and Output types to T? This could be more generic based on the Add implementation of T, e.g.:
| impl<T: Add<Output = T>> Add<Self> for Complex<T> { | |
| type Output = Complex<T::Output>; | |
| fn add(self, rhs: Self) -> Self::Output { | |
| Self::new(self.re + rhs.re, self.im + rhs.im) | |
| } | |
| } | |
| impl<T: Add<U>, U> Add<Complex<U>> for Complex<T> { | |
| type Output = Complex<T::Output>; | |
| fn add(self, rhs: Complex<U>) -> Self::Output { | |
| Complex::new(self.re + rhs.re, self.im + rhs.im) | |
| } | |
| } |
Similar thinking applies to the Sub<Self> implementation. The Add<T> and Sub<T> need RHS to be T to avoid overlapping trait impls, but Output doesn't need to be limited.
There was a problem hiding this comment.
That runs into a conflicting impl error. (it's important that Complex implements Copy for that error to show up)
There was a problem hiding this comment.
That makes sense. Just removing the <Output = T> shouldn't cause any issues since it's just an associated type (compiler explorer), so doing that seems worthwhile (e.g. this would allow usage with Add/Sub impls like those in rustc_apfloat)
There was a problem hiding this comment.
Sure, that seems fine. Fixed
There was a problem hiding this comment.
The <Output = T> on the other two impls could also be removed?
b05c4a3 to
5e3bab3
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
See num-complex implementation
5e3bab3 to
fb56ea5
Compare
|
Oof, it's been a while since university I guess, thanks. That removes the |
fb56ea5 to
d48bdef
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I believe you wanted more review work r? nia-e |
| impl<T: Add> Add<Self> for Complex<T> { | ||
| type Output = Complex<T::Output>; | ||
|
|
||
| fn add(self, rhs: Self) -> Self::Output { | ||
| Complex::new(self.re + rhs.re, self.im + rhs.im) | ||
| } | ||
| } |
There was a problem hiding this comment.
This really seems like it should be
| impl<T: Add> Add<Self> for Complex<T> { | |
| type Output = Complex<T::Output>; | |
| fn add(self, rhs: Self) -> Self::Output { | |
| Complex::new(self.re + rhs.re, self.im + rhs.im) | |
| } | |
| } | |
| impl<T: Add<U>, U> Add<Complex<U>> for Complex<T> { | |
| type Output = Complex<<T as Add<U>>::Output>; | |
| fn add(self, rhs: Self) -> Self::Output { | |
| Complex::new(self.re + rhs.re, self.im + rhs.im) | |
| } | |
| } |
since I see no reason to restrict it to T: Add<T>.
There was a problem hiding this comment.
That does not work because it conflicts with the implementation of add with a scalar right below
#[unstable(feature = "complex_numbers", issue = "154023")]
impl<T: Add<Output = T>> Add<T> for Complex<T> {
type Output = Complex<T::Output>;
fn add(self, rhs: T) -> Self::Output {
Complex::new(self.re + rhs, self.im)
}
}|
I'd quite like to build on top of this, it's all unstable surface area that can be re-litigated later, so I'd appreciate a review from someone here. |
|
eek, i've been a bit busy. I should be able to get to this in a day or two but if you need a faster review feel free to r? someone else |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@bors r+ |
implement `Add` and `Sub` for `Complex` tracking issue: rust-lang#154023 Adds the `Add` and `Sub` implementations described in the tracking issue. Some notes - I also added a derive for `Eq`, which is useful for `Complex<{integer}>` - The versions that add/sub by a scalar need a `Copy` bound. That seems fine for most actual use cases. Apparently `num_complex` will `Clone` in these operations https://docs.rs/num-complex/latest/num_complex/struct.Complex.html#impl-Add%3CT%3E-for-%26Complex%3CT%3E, but that seems unlike `core` to me. Anyhow, libs can re-litigate that later.
implement `Add` and `Sub` for `Complex` tracking issue: rust-lang#154023 Adds the `Add` and `Sub` implementations described in the tracking issue. Some notes - I also added a derive for `Eq`, which is useful for `Complex<{integer}>` - The versions that add/sub by a scalar need a `Copy` bound. That seems fine for most actual use cases. Apparently `num_complex` will `Clone` in these operations https://docs.rs/num-complex/latest/num_complex/struct.Complex.html#impl-Add%3CT%3E-for-%26Complex%3CT%3E, but that seems unlike `core` to me. Anyhow, libs can re-litigate that later.
…uwer Rollup of 12 pull requests Successful merges: - #161227 (implement `Add` and `Sub` for `Complex`) - #161280 (make target feature ABI check a hard error on ARM) - #161893 (Add custom allocator support to `(try_)map` on `UniqueArc` and `UniqueRc`) - #162154 (fix[154166]: closure debug capture print) - #161951 (Windows: add fallback if `canonicalize` fails) - #162173 (fix supposedly unreachable `bug!` being reachable) - #162180 (remove outdated next-solver handling) - #162191 (core: mark float `ClampBounds` methods as `#[inline]`) - #162195 (docs(time): clarify exact seconds for week and day) - #162199 (docs(time): clarify exact seconds for hour and minute) - #162222 (coverage: Small cleanups in `extract_hir_info`) - #162230 (Pass -Z merge-functions=disabled in tests/codegen-llvm/intrinsics/unchecked_math.rs)
…uwer Rollup of 12 pull requests Successful merges: - #161227 (implement `Add` and `Sub` for `Complex`) - #161280 (make target feature ABI check a hard error on ARM) - #161893 (Add custom allocator support to `(try_)map` on `UniqueArc` and `UniqueRc`) - #162154 (fix[154166]: closure debug capture print) - #161951 (Windows: add fallback if `canonicalize` fails) - #162173 (fix supposedly unreachable `bug!` being reachable) - #162180 (remove outdated next-solver handling) - #162191 (core: mark float `ClampBounds` methods as `#[inline]`) - #162195 (docs(time): clarify exact seconds for week and day) - #162199 (docs(time): clarify exact seconds for hour and minute) - #162222 (coverage: Small cleanups in `extract_hir_info`) - #162230 (Pass -Z merge-functions=disabled in tests/codegen-llvm/intrinsics/unchecked_math.rs)
Rollup merge of #161227 - folkertdev:complex-add-sub, r=nia-e implement `Add` and `Sub` for `Complex` tracking issue: #154023 Adds the `Add` and `Sub` implementations described in the tracking issue. Some notes - I also added a derive for `Eq`, which is useful for `Complex<{integer}>` - The versions that add/sub by a scalar need a `Copy` bound. That seems fine for most actual use cases. Apparently `num_complex` will `Clone` in these operations https://docs.rs/num-complex/latest/num_complex/struct.Complex.html#impl-Add%3CT%3E-for-%26Complex%3CT%3E, but that seems unlike `core` to me. Anyhow, libs can re-litigate that later.
View all comments
tracking issue: #154023
Adds the
AddandSubimplementations described in the tracking issue. Some notesEq, which is useful forComplex<{integer}>Copybound. That seems fine for most actual use cases.Apparently
num_complexwillClonein these operations https://docs.rs/num-complex/latest/num_complex/struct.Complex.html#impl-Add%3CT%3E-for-%26Complex%3CT%3E, but that seems unlikecoreto me. Anyhow, libs can re-litigate that later.