-
Notifications
You must be signed in to change notification settings - Fork 13.6k
some derive_more
refactors
#145145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
some derive_more
refactors
#145145
Conversation
6451c6d
to
2d2a6f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. The changes themselves look good. I do have a question re. if we want to add a comment about why we manually impl Eq
, so someone later doesn't try to "clean up" the manual Eq
impls... In any case, that's not really a blocking concern, so r=me with or without the comment.
@@ -78,6 +78,8 @@ struct WipGoalEvaluation<I: Interner> { | |||
pub result: Option<QueryResult<I>>, | |||
} | |||
|
|||
impl<I: Interner> Eq for WipGoalEvaluation<I> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion: should we leave a comment about why the manual Eq
derives somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it will be worth it to sprinkle a comment everywhere. People attempting to go back to derives might wonder why there were manual impls so they'd look at the blame.. Or the reviewer could look at the blame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope they do look at the blame... But yeah, good point about sprinkling them around. What I had in mind was having a single comment somewhere, but then it probably will just be never read again. So this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to return to using the derives instead of the manual impls once ModProg/derive-where#128 lands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better once my PR lands, yeah, so we could return to it, but unlike the built-in Eq
derive which uses a single type AssertParamIsEq
across all derives, the derive-where
impl creates a new struct
every time you derive Eq
, so I am still not completely happy about it
@rustbot author |
@bors r+ rollup |
This PR now has merge conflicts, but bors hasn't noticed yet. @bors r- |
some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort
2d2a6f6
to
71f6e53
Compare
@bors r=jieyouxu |
Rollup of 8 pull requests Successful merges: - #144739 (Use new public libtest `ERROR_EXIT_CODE` constant in rustdoc) - #145089 (Improve error output when a command fails in bootstrap) - #145112 ([win][arm64ec] Partial fix for raw-dylib-link-ordinal on Arm64EC) - #145135 (Stabilize `duration_constructors_lite` feature) - #145146 (remove `P`) - #145152 (Use `to_ascii_lowercase` to avoid heap alloc in `detect_confuse_type`) - #145156 (Override custom Cargo `build-dir` in bootstrap) - #145160 (Change days-threshold to 28 in [behind-upstream]) Failed merges: - #145145 (some `derive_more` refactors) r? `@ghost` `@rustbot` modify labels: rollup
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
Rollup of 19 pull requests Successful merges: - #141624 (unstable-book: Add stubs for environment variables; document some of the important ones) - #143093 (Simplify polonius location-sensitive analysis) - #144402 (Stabilize loongarch32 inline asm) - #144403 (`tests/ui/issues/`: The Issues Strike Back [4/N]) - #144544 (Start reporting future breakage for `ILL_FORMED_ATTRIBUTE_INPUT` in dependencies) - #144739 (Use new public libtest `ERROR_EXIT_CODE` constant in rustdoc) - #145089 (Improve error output when a command fails in bootstrap) - #145112 ([win][arm64ec] Partial fix for raw-dylib-link-ordinal on Arm64EC) - #145129 ([win][arm64ec] Add `/machine:arm64ec` when linking LLVM as Arm64EC) - #145130 (improve "Documentation problem" issue template.) - #145135 (Stabilize `duration_constructors_lite` feature) - #145145 (some `derive_more` refactors) - #145147 (rename `TraitRef::from_method` to `from_assoc`) - #145156 (Override custom Cargo `build-dir` in bootstrap) - #145160 (Change days-threshold to 28 in [behind-upstream]) - #145162 (`{BTree,Hash}Map`: add "`Entry` API" section heading) - #145175 (Enable limit_rdylib_exports on Solaris) - #145187 (Fix an unstable feature comment that wasn't a doc comment) - #145191 (`suggest_borrow_generic_arg`: use the correct generic args) r? `@ghost` `@rustbot` modify labels: rollup
…r, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
Rollup of 18 pull requests Successful merges: - #141624 (unstable-book: Add stubs for environment variables; document some of the important ones) - #143093 (Simplify polonius location-sensitive analysis) - #144402 (Stabilize loongarch32 inline asm) - #144403 (`tests/ui/issues/`: The Issues Strike Back [4/N]) - #144739 (Use new public libtest `ERROR_EXIT_CODE` constant in rustdoc) - #145089 (Improve error output when a command fails in bootstrap) - #145112 ([win][arm64ec] Partial fix for raw-dylib-link-ordinal on Arm64EC) - #145129 ([win][arm64ec] Add `/machine:arm64ec` when linking LLVM as Arm64EC) - #145130 (improve "Documentation problem" issue template.) - #145135 (Stabilize `duration_constructors_lite` feature) - #145145 (some `derive_more` refactors) - #145147 (rename `TraitRef::from_method` to `from_assoc`) - #145156 (Override custom Cargo `build-dir` in bootstrap) - #145160 (Change days-threshold to 28 in [behind-upstream]) - #145162 (`{BTree,Hash}Map`: add "`Entry` API" section heading) - #145175 (Enable limit_rdylib_exports on Solaris) - #145187 (Fix an unstable feature comment that wasn't a doc comment) - #145191 (`suggest_borrow_generic_arg`: use the correct generic args) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 17 pull requests Successful merges: - #141624 (unstable-book: Add stubs for environment variables; document some of the important ones) - #143093 (Simplify polonius location-sensitive analysis) - #144402 (Stabilize loongarch32 inline asm) - #144403 (`tests/ui/issues/`: The Issues Strike Back [4/N]) - #144739 (Use new public libtest `ERROR_EXIT_CODE` constant in rustdoc) - #145089 (Improve error output when a command fails in bootstrap) - #145112 ([win][arm64ec] Partial fix for raw-dylib-link-ordinal on Arm64EC) - #145129 ([win][arm64ec] Add `/machine:arm64ec` when linking LLVM as Arm64EC) - #145130 (improve "Documentation problem" issue template.) - #145135 (Stabilize `duration_constructors_lite` feature) - #145145 (some `derive_more` refactors) - #145147 (rename `TraitRef::from_method` to `from_assoc`) - #145156 (Override custom Cargo `build-dir` in bootstrap) - #145160 (Change days-threshold to 28 in [behind-upstream]) - #145162 (`{BTree,Hash}Map`: add "`Entry` API" section heading) - #145187 (Fix an unstable feature comment that wasn't a doc comment) - #145191 (`suggest_borrow_generic_arg`: use the correct generic args) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 17 pull requests Successful merges: - #141624 (unstable-book: Add stubs for environment variables; document some of the important ones) - #143093 (Simplify polonius location-sensitive analysis) - #144402 (Stabilize loongarch32 inline asm) - #144403 (`tests/ui/issues/`: The Issues Strike Back [4/N]) - #144739 (Use new public libtest `ERROR_EXIT_CODE` constant in rustdoc) - #145089 (Improve error output when a command fails in bootstrap) - #145112 ([win][arm64ec] Partial fix for raw-dylib-link-ordinal on Arm64EC) - #145129 ([win][arm64ec] Add `/machine:arm64ec` when linking LLVM as Arm64EC) - #145130 (improve "Documentation problem" issue template.) - #145135 (Stabilize `duration_constructors_lite` feature) - #145145 (some `derive_more` refactors) - #145147 (rename `TraitRef::from_method` to `from_assoc`) - #145156 (Override custom Cargo `build-dir` in bootstrap) - #145160 (Change days-threshold to 28 in [behind-upstream]) - #145162 (`{BTree,Hash}Map`: add "`Entry` API" section heading) - #145187 (Fix an unstable feature comment that wasn't a doc comment) - #145191 (`suggest_borrow_generic_arg`: use the correct generic args) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #145145 - fee1-dead-contrib:push-qnmpmtmtpkkr, r=jieyouxu some `derive_more` refactors some clauses can be merged together without requiring an attribute for each trait derived. also manually impl `Eq` because the `derive_where` generated code is too much for my comfort (cc ModProg/derive-where#128)
some clauses can be merged together without requiring an attribute for each trait derived.
also manually impl
Eq
because thederive_where
generated code is too much for my comfort (cc ModProg/derive-where#128)