Implement core ops for references + fix core module documentation #21227
Conversation
* Not all traits are part of the prelude anymore * We switched from pass-by-reference to pass-by-value for most traits * Add some explanations around pass-by-value traits in the context of generic code and additional implementations for reference types.
|
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
| // based on "T + U" where T and U are expected `Copy`able | ||
| macro_rules! forward_ref_binop { | ||
| (impl $imp:ident, $method:ident for $t:ty, $u:ty) => { | ||
| forward_ref_val_binop! { impl $imp, $method for $t, $u } |
There was a problem hiding this comment.
I wonder if these three macros could be inlined and just have 3 impl blocks in here.
There was a problem hiding this comment.
@huonw Sure, that's doable. I've never used them anywhere except in the bottom macro. And I think it should save 12 non-empty lines of code. Do you want me to inline them? Personally, I don't have a preference.
There was a problem hiding this comment.
Yeah, I think they should be inlined; thanks!
|
@huonw updated macros as suggested |
|
I checked libcoretest/ops.rs and didn't see any tests for these kinds of implementations. Otherwise I would have added tests there already, too. But I think it might be a good idea to add tests to make sure that all the operators are doing what they are supposed to and that we don't have a "copy-&-paste" bug that, for example, implements Mul in terms of + or something like that. I'm thinking about adding tests that at least check whether a + b = &a + b = a + &b = &a + &b (repeated with all other binary operators, etc and for all built-in type combinations for which these implementations are provided). |
|
Thanks for this PR, and sorry for the slow review (it was a holiday weekend). This looks great. My only request is to annotate with |
|
@aturon Done. |
|
@sellibitze Somehow I missed the comment about testing before. I'm ok taking this PR as-is, but would also be delighted if you wanted to add some tests like the ones you suggested. Should I wait for those before sending to bors? |
|
@aturon I don't yet have such tests implemented because I waited for feedback. So, it would take a while to get this done. I'm ok with adding them to this or a new PR. |
|
@sellibitze OK, in that case I think let's move forward with the PR as it stands, and some tests can be brought in as a follow-up PR. (In general we like to land tests together with new code, but in a case like this it's totally fine.) Thanks again for doing this! |
|
⌛ Testing commit 970fd74 with merge 8abcbab... |
As discussed with @aturon I added implementations of various op traits for references to built-in types which was already suggested by the ops reform RFC. The 2nd commit updates the module documentation of core::ops to fully reflect the recent change from pass-by-reference to pass-by-value and expands on the implications for generic code.
As discussed with @aturon I added implementations of various op traits for references to built-in types which was already suggested by the ops reform RFC.
The 2nd commit updates the module documentation of core::ops to fully reflect the recent change from pass-by-reference to pass-by-value and expands on the implications for generic code.