Skip to content
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

Closed
Centril opened this issue Jul 5, 2019 · 11 comments · Fixed by #63127
Closed

Cleanup: Consistently use Param instead of Arg #62426

Centril opened this issue Jul 5, 2019 · 11 comments · Fixed by #63127
Assignees
Labels
A-attributes Area: #[attributes(..)] A-frontend Area: frontend (errors, parsing and HIR) A-parser Area: The parsing of Rust source code to an AST. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Jul 5, 2019

For example, ideally fn visit_arg would be named fn visit_param since this is about formal parameters (in function signatures and from the POV inside the function body) instead of actual arguments (the values you pass to functions).

The type Arg should probably be named Param.

(This is not an exhaustive list, there are other places "arg" is used not mentioned here that should probably use "param" instead)

cc @petrochenkov @c410-f3r
cc #61856

This issue has been assigned to @kper via this comment.

@Centril Centril added A-frontend Area: frontend (errors, parsing and HIR) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-attributes Area: #[attributes(..)] A-parser Area: The parsing of Rust source code to an AST. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 5, 2019
@kper
Copy link
Contributor

kper commented Jul 8, 2019

Can I work on this?

@rust-lang rust-lang deleted a comment from rustbot Jul 8, 2019
@rustbot
Copy link
Collaborator

rustbot commented Jul 8, 2019

Error: Only Rust team members can assign other users

Please let @rust-lang/release know if you're having trouble with this bot.

@csmoe
Copy link
Member

csmoe commented Jul 8, 2019

@kper you can assign to yourself with @rustbot claim.

@rustbot rustbot self-assigned this Jul 8, 2019
@kper
Copy link
Contributor

kper commented Jul 8, 2019

@rustbot kper

@csmoe
Copy link
Member

csmoe commented Jul 8, 2019

@kper it's already "assign"ed to you.

@kper
Copy link
Contributor

kper commented Jul 8, 2019

I tried to begin with the renaming and I have been wondering how aggressive I should rename? It is kind of hard for me to see when I have real arguments or parameters. My first try (Commit 6c9051c as referenced above) led to extremely a lot of change. Can I have some feedback?

In addition, I noticed that the issue has a "blocked" label. Should I therefore wait until #61856 is finished?

@Centril
Copy link
Contributor Author

Centril commented Jul 8, 2019

Should I therefore wait until #61856 is finished?

I think that's a good idea.

It is kind of hard for me to see when I have real arguments or parameters. My first try (Commit 6c9051c as referenced above) led to extremely a lot of change. Can I have some feedback?

This looks largely right, tho I didn't look at parser.rs. In general, if something refers to things a function accepts rather than the expressions (or types when it's about type args) passed into them, then they are parameters and not arguments.

Please also avoid running rustfmt on the code as that will make everyone else have to rebase and it makes it harder to review the actual changes.

@c410-f3r
Copy link
Contributor

#61856 has been merged

@kper
Copy link
Contributor

kper commented Jul 29, 2019

@Centril can you look over my commit? :)

@Centril
Copy link
Contributor Author

Centril commented Jul 29, 2019

@kper Can you file it as a PR? Easier to review that way.

@kper
Copy link
Contributor

kper commented Jul 30, 2019

@Centril sure :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] A-frontend Area: frontend (errors, parsing and HIR) A-parser Area: The parsing of Rust source code to an AST. C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants