Generalize binary operators #23673
Generalize binary operators #23673
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
hmm the rebase is encountering a problem, I may push a tweaked version soon |
|
Huzzah! |
|
OK, things are looking good locally. I'll push a revised version shortly that passes make check. |
nikomatsakis
force-pushed
the
issue-23319-binops-ng-5
branch
2 times, most recently
from
1c310fb
to
e4fcdea
Compare
|
For some odd reason I see a failure locally in the |
nikomatsakis
force-pushed
the
issue-23319-binops-ng-5
branch
from
e4fcdea
to
82fdbdb
Compare
|
oh and cc @eddyb -- this changed the flow around coercions a bit, though nothing that should really affect you |
|
@bors try |
The current binary operator code assumed that if the LHS was a scalar (`i32` etc), then the RHS had to match. This is not true with multidispatch. This PR generalizes the existing code to (primarily) use the traits -- this also allows us to defer the precise type-checking when the types aren't fully known. The one caveat is unstable SIMD types, which don't fit in with the current traits -- in that case, the types must be known ahead of time. There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth. cc @japaric cc @aturon Fixes #23319 (and others).
|
💔 Test failed - try-bsd |
|
☔ The latest upstream changes (presumably #23681) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
force-pushed
the
issue-23319-binops-ng-5
branch
2 times, most recently
from
40f1d55
to
5265d57
Compare
| map.insert(i, i); | ||
| } | ||
|
|
||
| // measure | ||
| b.iter(|| { | ||
| let k = rng.gen() % n; | ||
| let k = rng.gen::<usize>() % n; |
There was a problem hiding this comment.
This should likely not be usize.
There was a problem hiding this comment.
Hmm, I presume because that would alter what is being tested when run on 32-bit and 64-bit systems? Makes sense. I can change to u32.
nikomatsakis
force-pushed
the
issue-23319-binops-ng-5
branch
from
5265d57
to
1dbddda
Compare
|
@nikomatsakis don't run the |
|
@eddyb ah, that might indeed be the problem... |
|
We should consider trying to remove that fragility, perhaps in a similar manner that |
| @@ -23,4 +23,6 @@ fn main() { | |||
| unsafe { libc::exit(0 as libc::c_int); } | |||
| }); | |||
| 2_usize + (loop {}); | |||
| //~^ ERROR E0277 | |||
There was a problem hiding this comment.
Hmm. I don't think you should have moved this test into compile-fail.
Were you intending to make a copy of it?
Could you fix the test by explicitly forcing the type of the looping expression, e.g.:
(2 + { let x: i8 = loop { }; x });There was a problem hiding this comment.
(this was supposed to be a comment on commit f8a658c ; maybe its resolved in follow-on commits.)
There was a problem hiding this comment.
(of course it might be even nicer to not require the type annotation at all, since after all the addition is an unreachable expression in the control flow. but I'd be satisfied with an annotation rather than trying to go further.)
There was a problem hiding this comment.
ok, let me try that
|
@nikomatsakis okay. Overall I think this is a nice cleanup compared to the previous code. its mostly just a bunch of questions from me. Along with a few things that might have been mistakes on your part; hopefully its clear from the comments. You can r=me (preferably after addressing the review notes, though). |
|
@pnkfelix the problem with lang-item-public had to do with the fact that there are no impls for Add, so you get type errors in the end iirc. I did debate about that for a while. I guess I could bring in the Add trait to keep the original code. |
|
☔ The latest upstream changes (presumably #23796) made this pull request unmergeable. Please resolve the merge conflicts. |
the plan is to treat all binary operators as if they were overloaded, relying on the fact that we have impls for all the builtin scalar operations (and no more). But then during writeback we clear the overload if the types correspond to a builtin op. This strategy allows us to avoid having to know the types of the operands ahead of time. It also avoids us overspecializing as we did in the past.
surprising casts. This version more obviously corresponds to the builtin semantics.
This is due to a [breaking-change] to operators. The primary affected code is uses of the `Rng` trait where we used to (incorrectly) infer the right-hand-side type from the left-hand-side, in the case that the LHS type was a scalar like `i32`. The fix is to add a type annotation like `x + rng.gen::<i32>()`.
Fixes rust-lang#22743. Fixes rust-lang#19035. Fixes rust-lang#22099. (Those all seem to be exactly the same scenario.)
nikomatsakis
force-pushed
the
issue-23319-binops-ng-5
branch
from
1dbddda
to
7595c25
Compare
|
@bors r=pnkfelix 7595c25 |
|
⌛ Testing commit 7595c25 with merge 9de34a8... |
The current binary operator code assumed that if the LHS was a scalar (`i32` etc), then the RHS had to match. This is not true with multidispatch. This PR generalizes the existing code to (primarily) use the traits -- this also allows us to defer the precise type-checking when the types aren't fully known. The one caveat is the unstable SIMD types, which don't fit in with the current traits -- in that case, the LHS type must be known to be SIMD ahead of time. There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth. cc @japaric cc @aturon Fixes #23319 (and others).
|
This necessitated extra type annotations in the |
|
Oh, I guess it must be, since c92bdcb made the same change to rustc’s internal copy of |
Resolves this error, which is fallout from rust-lang/rust#23673: src/structs/dmat.rs:501:43: 501:74 error: type annotations required: cannot resolve `<f64 as core::ops::Div<_>>::Output == f64` [E0284] src/structs/dmat.rs:501 let normalizer: N = Cast::from(1.0f64 / Cast::from(self.nrows)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Anders Kaseorg <[email protected]>
Resolves this error, which is fallout from rust-lang/rust#23673: src/structs/dmat.rs:501:43: 501:74 error: type annotations required: cannot resolve `<f64 as core::ops::Div<_>>::Output == f64` [E0284] src/structs/dmat.rs:501 let normalizer: N = Cast::from(1.0f64 / Cast::from(self.nrows)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Anders Kaseorg <[email protected]>
Type inference with operators was changed in rust-lang/rust#23673.
The current binary operator code assumed that if the LHS was a scalar (
i32etc), then the RHS had to match. This is not true with multidispatch. This PR generalizes the existing code to (primarily) use the traits -- this also allows us to defer the precise type-checking when the types aren't fully known. The one caveat is the unstable SIMD types, which don't fit in with the current traits -- in that case, the LHS type must be known to be SIMD ahead of time.There is one semi-hacky bit in that during writeback, for builtin operators, if the types resolve to scalars (i32 etc) then we clear the method override. This is because we know what the semantics are and it is more efficient to generate the code directly. It also ensures that we can use these overloaded operators in constants and so forth.
cc @japaric
cc @aturon
Fixes #23319 (and others).