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

Fix false positive in write_literal and print_literal (numeric literals) #6408

Merged
merged 6 commits into from
Jan 21, 2021
Merged

Fix false positive in write_literal and print_literal (numeric literals) #6408

merged 6 commits into from
Jan 21, 2021

Conversation

pro-grammer1
Copy link
Contributor

@pro-grammer1 pro-grammer1 commented Dec 1, 2020

changelog: No longer lint numeric literals in [write_literal] and [print_literal].

Fixes #6335

@rust-highfive
Copy link

r? @ebroto

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 1, 2020
@pro-grammer1
Copy link
Contributor Author

Fixes #6335

@pro-grammer1 pro-grammer1 marked this pull request as ready for review December 1, 2020 15:40
@pro-grammer1 pro-grammer1 changed the title Fix false positive in write_literal and print_literal due to numeric literals Fix false positive in write_literal and print_literal (numeric literals) Dec 1, 2020
@rustbot
Copy link
Collaborator

rustbot commented Dec 1, 2020

Error: The feature major_change is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

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

Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits.

Also, the reference file for the tests needs to be updated because the stderr files have changed, as we are not linting if the literals are ints/floats. To do this, you can run the tests/ui/update-all-references.sh script.

Comment on lines 446 to 450
ExprKind::Lit(lit)
if match lit.kind {
LitKind::Int(_, _) | LitKind::Float(_, _) => false,
_ => true,
} =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ExprKind::Lit(lit)
if match lit.kind {
LitKind::Int(_, _) | LitKind::Float(_, _) => false,
_ => true,
} =>
ExprKind::Lit(lit) if !matches!(lit.kind, LitKind::Int(..) | LitKind::Float(..)) => {

We can simplify using matches!()

Comment on lines 473 to 476
if match lit.kind {
LitKind::Int(_, _) | LitKind::Float(_, _) => false,
_ => true,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if match lit.kind {
LitKind::Int(_, _) | LitKind::Float(_, _) => false,
_ => true,
};
if !matches!(lit.kind, LitKind::Int(..) | LitKind::Float(..));

Same here

@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 8, 2020
@giraffate
Copy link
Contributor

ping from triage @pro-grammer1. There seems to be some fixes left to be done. Do you have any questions on how to preceed here?

@giraffate
Copy link
Contributor

ping from triage @pro-grammer1. According to the triage procedure, I close this because there has no activity in 2 weeks from previous ping. If you have more time to work on this, feel free to reopen.

@giraffate giraffate closed this Jan 5, 2021
@giraffate giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 5, 2021
@pro-grammer1
Copy link
Contributor Author

I've made the changes, so I would like to reopen this.

@giraffate
Copy link
Contributor

Thanks for taking time to work on this! I reopen this.

@giraffate giraffate reopened this Jan 17, 2021
@giraffate giraffate added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-inactive-closed Status: Closed due to inactivity labels Jan 17, 2021
@pro-grammer1
Copy link
Contributor Author

I've made the suggested changes and updated the tests. Is there anything else I should do?

@oli-obk oli-obk assigned oli-obk and unassigned ebroto Jan 21, 2021
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oli was faster, but I have a comment anyway 😄

--> $DIR/ice-3891.rs:2:5
|
LL | 1x;
| ^^ invalid suffix `x`
|
= help: the suffix must be one of the integral types (`u32`, `isize`, etc)
= help: the suffix must be one of the numeric types (`u32`, `isize`, `f32`, etc.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? This is a rustc lint and should be unaffected by a Clippy PR. (rebase gone wrong?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering the same thing... but since CI is passing... I guess this is the lack of a rebase? (master has this change already) Like this change will go away when rebased over master. Though that does make me wonder how git managed to not treat this as a conflict... I guess because the change is exactly the same.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 21, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 21, 2021

📌 Commit 32b2a3f has been approved by oli-obk

@bors
Copy link
Collaborator

bors commented Jan 21, 2021

⌛ Testing commit 32b2a3f with merge 7b50a4e...

@bors
Copy link
Collaborator

bors commented Jan 21, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: oli-obk
Pushing 7b50a4e to master...

@bors bors merged commit 7b50a4e into rust-lang:master Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive: print_literal with numeric literal arguments
8 participants