-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Change the return type of Ty::kind
from &TyKind
to TyKind
.
#126069
Conversation
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred in compiler/rustc_codegen_gcc Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt changes to the core type system Some changes occurred in match lowering cc @Nadrieril The Miri subtree was changed cc @rust-lang/miri changes to the core type system Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor Some changes occurred in cc @BoxyUwU Some changes occurred in match checking cc @Nadrieril changes to the core type system Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in exhaustiveness checking cc @Nadrieril HIR ty lowering was modified cc @fmease Some changes occurred in compiler/rustc_sanitizers cc @rust-lang/project-exploit-mitigations, @rcvalle Some changes occurred in need_type_info.rs cc @lcnr |
This comment has been minimized.
This comment has been minimized.
this needs perf. I remember trying this previously and getting a performance regression |
Ty::kind
from TyKind
to &TyKind
.Ty::kind
from &TyKind
to TyKind
.
The commit message still describes the change the wrong way around (lcnr fixed it in the PR title). I got quite confused due to that when reading the notification email. :) |
I really wish it was possible to mark a PR as a draft while creating it and not notifying everybody until it's in a non-draft state. |
50a3cde
to
25e854a
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Change the return type of `Ty::kind` from `&TyKind` to `TyKind`. This is valid because `TyKind` impls `Copy`, and makes `Ty` consistent with `Predicate` and `Region`, both of which have `kind` methods that return a non-reference. The change removes more than one thousand `&` and `*` sigils, while requiring the addition of just five! It's clearly moving with the grain of the existing code. Almost all of the removed sigils fit one of the following three patterns. - Remove the dereference in the match expression: `match *ty.kind() { ... }` -> `match ty.kind() { ... }` - Remove the dereference in the match pattern: `match ty.kind() { &ty::Foo(foo) => { ... foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` - Remove the derefernce in the match arm: `match ty.kind() { ty::Foo(foo) => { ... *foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` A couple of other methods had their signatures changed. - `TypeErrCtxt::cmp_fn_sig`: `&` removed from two args. - `EncodableWithShorthand::variant`: `&` removed from return type. r? `@ghost`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b3b9397): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary 3.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 672.761s -> 673.707s (0.14%) |
I think you can create easily make draft PR but rustbot won't distinguish it from "ready PR", or at least that used to be the case. |
25e854a
to
fa2b1a9
Compare
going through my list of assigned PRs today. @rustbot author |
This comment has been minimized.
This comment has been minimized.
fa2b1a9
to
9b3054f
Compare
This comment has been minimized.
This comment has been minimized.
This is valid because `TyKind` impls `Copy`, and makes `Ty` consistent with `Predicate` and `Region`, both of which have `kind` methods that return a non-reference. The change removes more than one thousand `&` and `*` sigils, while requiring the addition of just five! It's clearly moving with the grain of the existing code. Almost all of the removed sigils fit one of the following three patterns. - Remove the dereference in the match expression: `match *ty.kind() { ... }` -> `match ty.kind() { ... }` - Remove the dereference in the match pattern: `match ty.kind() { &ty::Foo(foo) => { ... foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` - Remove the derefernce in the match arm: `match ty.kind() { ty::Foo(foo) => { ... *foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` A couple of other methods had their signatures changed. - `TypeErrCtxt::cmp_fn_sig`: `&` removed from two args. - `EncodableWithShorthand::variant`: `&` removed from return type.
9b3054f
to
bd3667d
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Change the return type of `Ty::kind` from `&TyKind` to `TyKind`. This is valid because `TyKind` impls `Copy`, and makes `Ty` consistent with `Predicate` and `Region`, both of which have `kind` methods that return a non-reference. The change removes more than one thousand `&` and `*` sigils, while requiring the addition of just five! It's clearly moving with the grain of the existing code. Almost all of the removed sigils fit one of the following three patterns. - Remove the dereference in the match expression: `match *ty.kind() { ... }` -> `match ty.kind() { ... }` - Remove the dereference in the match pattern: `match ty.kind() { &ty::Foo(foo) => { ... foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` - Remove the derefernce in the match arm: `match ty.kind() { ty::Foo(foo) => { ... *foo ... }` -> `match ty.kind() { ty::Foo(foo) => { ... foo ... }` A couple of other methods had their signatures changed. - `TypeErrCtxt::cmp_fn_sig`: `&` removed from two args. - `EncodableWithShorthand::variant`: `&` removed from return type. r? `@ghost`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (342c2c0): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (secondary -3.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 751.474s -> 753.088s (0.21%) |
…, r=jieyouxu Add a comment explaining the return type of `Ty::kind`. At least we'll get a useful comment out of rust-lang#126069 :) r? `@lcnr`
…, r=jieyouxu Add a comment explaining the return type of `Ty::kind`. At least we'll get a useful comment out of rust-lang#126069 :) r? ``@lcnr``
…, r=jieyouxu Add a comment explaining the return type of `Ty::kind`. At least we'll get a useful comment out of rust-lang#126069 :) r? ```@lcnr```
…, r=jieyouxu Add a comment explaining the return type of `Ty::kind`. At least we'll get a useful comment out of rust-lang#126069 :) r? ````@lcnr````
Rollup merge of rust-lang#129110 - nnethercote:Ty-kind-ret-ty-comment, r=jieyouxu Add a comment explaining the return type of `Ty::kind`. At least we'll get a useful comment out of rust-lang#126069 :) r? ````@lcnr````
This is valid because
TyKind
implsCopy
, and makesTy
consistent withPredicate
andRegion
, both of which havekind
methods that return a non-reference.The change removes more than one thousand
&
and*
sigils, while requiring the addition of just five! It's clearly moving with the grain of the existing code.Almost all of the removed sigils fit one of the following three patterns.
Remove the dereference in the match expression:
match *ty.kind() { ... }
->match ty.kind() { ... }
Remove the dereference in the match pattern:
match ty.kind() { &ty::Foo(foo) => { ... foo ... }
->match ty.kind() { ty::Foo(foo) => { ... foo ... }
Remove the derefernce in the match arm:
match ty.kind() { ty::Foo(foo) => { ... *foo ... }
->match ty.kind() { ty::Foo(foo) => { ... foo ... }
A couple of other methods had their signatures changed.
TypeErrCtxt::cmp_fn_sig
:&
removed from two args.EncodableWithShorthand::variant
:&
removed from return type.r? @ghost