-
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
Cleanup: Consistently use Param
instead of Arg
#62426
#63127
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This comment has been minimized.
This comment has been minimized.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Is there anything left to change? |
☔ The latest upstream changes (presumably #63207) made this pull request unmergeable. Please resolve the merge conflicts. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #63234) made this pull request unmergeable. Please resolve the merge conflicts. |
cc @rust-lang/compiler I haven't done a full review, but AFAICT the only blocker is that I'm a bit worried about calling value parameters just " |
Maybe |
Seems ok as is, not worse than just " |
@Centril do you mean to rename |
@kper I basically agree with @petrochenkov that being more specific is not necessary; it should be clear from context if it is about values or types or whatnot. |
@eddyb Might be worth disambiguating in those specific cases? |
☔ The latest upstream changes (presumably #63437) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc/hir/lowering.rs
Outdated
}) | ||
.collect() | ||
} | ||
|
||
// Lowers a function declaration. | ||
// | ||
// `decl`: the unlowered (AST) function declaration. | ||
// `fn_def_id`: if `Some`, impl Trait arguments are lowered into generic parameters on the | ||
// `fn_def_id`: if `Some`, impl Trait parameters are lowered into generic parameters on the |
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.
An example where parameters
is used twice in the same place to refer to both value parameters and generic parameters.
src/librustc/hir/lowering.rs
Outdated
// only look at the lifetime parameters introduced by the arguments. | ||
let lifetime_count_before_args = self.lifetimes_to_define.len(); | ||
// only look at the lifetime parameters introduced by the parameters. | ||
let lifetime_count_before_params = self.lifetimes_to_define.len(); |
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.
Potentially confusing because of the existence of "lifetime parameters".
☔ The latest upstream changes (presumably #63878) made this pull request unmergeable. Please resolve the merge conflicts. |
✌️ @kper can now approve this pull request |
📌 Commit 3c18dce0414d63d765e23e420afe8b8d5b05eadd has been approved by `nikomatsakis`` |
@bors r- Sorry, bors, you weren't meant to detect that. This PR won't merge you silly goose. |
Simple rebase won't help here. Please drop those changes when resolving conflicts. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@mati865 thank you, you're right I totally messed up with those changes.
|
@kper if failed tests contain |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
`async-await/no-args-non-move-async-closure` `generator/no-arguments-on-generators`
@bors r=nikomatsakis |
📌 Commit 97319b2 has been approved by |
☀️ Test successful - checks-azure |
📣 Toolstate changed by #63127! Tested on commit bbd48e6. 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). |
Tested on commit rust-lang/rust@bbd48e6. Direct link to PR: <rust-lang/rust#63127> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
Replace `Arg` with `Param` Fix build issue. Rustup to rust-lang/rust#63127 changelog: none
Fixes #62426