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.

Sign up for GitHub

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

Jump to bottom

Hex bin digit grouping #6183

Merged
merged 5 commits into from
Oct 25, 2020
Merged

Hex bin digit grouping #6183

merged 5 commits into from
Oct 25, 2020

Conversation

cgm616
Copy link
Contributor

@cgm616 cgm616 commented Oct 16, 2020 •
edited by flip1995
Loading

This revives and updates an old pr (#3391) for the current API.

Closes #2538.

Please keep the line below
changelog: Add [unusual_byte_groupings] lint.

@rust-highfive
Copy link

r? @flip1995

(rust_highfive has picked a reviewer for you, use r? to override)

@cgm616 cgm616 marked this pull request as ready for review October 16, 2020 19:33
@cgm616 cgm616 force-pushed the hex_bin_digit_grouping branch from aee3baf to 2d0d6ea Compare October 16, 2020 21:22
@cgm616
Copy link
Contributor Author

cgm616 commented Oct 16, 2020 •
edited
Loading

I think this PR is all set now. There was one change made to the suggested grouping for hexadecimal literals that might need discussion. Instead of groups of 4, like 0xDEAD_BEEF, clippy will now suggest groups of two, like 0xDE_AD_BE_EF. This is to make sure that clippy's fixes won't contravene this lint, which ensures byte-sized chunks for hex literals.

flip1995
flip1995 reviewed Oct 17, 2020
View reviewed changes
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.

I really don't like the suggestion change of the other lint. IMO splitting hex numbers in groups of 4 is more accepted than groups of 2.

clippy_lints/src/literal_representation.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Oct 17, 2020
@cgm616
Copy link
Contributor Author

cgm616 commented Oct 17, 2020

I really don't like the suggestion change of the other lint. IMO splitting hex numbers in groups of 4 is more accepted than groups of 2.

Yeah, that's fair. I just reverted that change and switched this new lint to checking if hex literals are in groups of four to stay consistent.

@cgm616 cgm616 force-pushed the hex_bin_digit_grouping branch 3 times, most recently from db60db7 to f865bd0 Compare October 22, 2020 01:23
@cgm616 cgm616 requested a review from flip1995 October 24, 2020 16:05
flip1995
flip1995 requested changes Oct 25, 2020
View reviewed changes
tests/ui/literals.rs Outdated Show resolved Hide resolved
tests/ui/literals.stderr Outdated Show resolved Hide resolved
@cgm616 cgm616 force-pushed the hex_bin_digit_grouping branch from f865bd0 to f5a88b6 Compare October 25, 2020 13:18
@cgm616
Copy link
Contributor Author

cgm616 commented Oct 25, 2020

For some reason, my local builds always fail with seven or so of this error:

no variant or associated item named `ConstBlock` found for enum `rustc_ast::ExprKind`

Does anyone know why? It makes debugging this lint really hard since I can't run any of the tests.

@ebroto
Copy link
Member

ebroto commented Oct 25, 2020

my local builds always fail with seven or so of this error

I think you need to run setup-toolchain.sh again to catch up with the supported rustc version. See doc/basics.md.

@cgm616
Copy link
Contributor Author

cgm616 commented Oct 25, 2020

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

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

cgm616 commented Oct 25, 2020

my local builds always fail with seven or so of this error

I think you need to run setup-toolchain.sh again to catch up with the supported rustc version. See doc/basics.md.

That helps partially; thank you. Now, though, I have a bunch of errors about crates being built with incompatible versions of rustc. I've tried deleting the target/ folder and running cargo clean, but neither work.

@ebroto
Copy link
Member

ebroto commented Oct 25, 2020

I have a bunch of errors about crates being built with incompatible versions of rustc

Could you share the errors?

Also, make sure you rebased on top of the current master.

@cgm616
Copy link
Contributor Author

cgm616 commented Oct 25, 2020

Could you share the errors?

They went away mysteriously after waiting a while. It's possible that rust-analyzer running in VSCode had something to do with it. Thanks for the help!

@flip1995
Copy link
Member

It's possible that rust-analyzer running in VSCode had something to do with it.

I have the "rust-analyzer.checkOnSave.extraArgs": ["--target-dir", "target/rust-analyzer"], config option set for rust-analyzer (I use coc.nvim, but I'm sure there's something similar for VSCode). This will create a rust-analyzer subdir in the target dir (for every project), so no more conflicting cargo build and rust-analyzer builds. I only suggest to use that if you have enough memory to spare.

flip1995
flip1995 reviewed Oct 25, 2020
View reviewed changes
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.

Two small things left, before this is ready to get merged.

tests/ui/literals.rs Outdated Show resolved Hide resolved
clippy_lints/src/literal_representation.rs Outdated Show resolved Hide resolved
@flip1995 flip1995 changed the title [WIP] Hex bin digit grouping Hex bin digit grouping Oct 25, 2020
@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 25, 2020
@flip1995
Copy link
Member

Thanks! Waiting on rustup.

@cgm616 I removed the WIP from the title. Was there something you thought was still missing?

@cgm616
Copy link
Contributor Author

cgm616 commented Oct 25, 2020

@cgm616 I removed the WIP from the title. Was there something you thought was still missing?

Nope! Just leftover from the beginning. Thanks for everything!

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Oct 25, 2020

📌 Commit 312bbff has been approved by flip1995

@bors
Copy link
Contributor

bors commented Oct 25, 2020

⌛ Testing commit 312bbff with merge eceebc3...

@bors
Copy link
Contributor

bors commented Oct 25, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing eceebc3 to master...

@bors bors merged commit eceebc3 into rust-lang:master Oct 25, 2020
@HKalbasi HKalbasi mentioned this pull request Jul 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebroto ebroto ebroto left review comments

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Underscores only every 2^n digits in non-decimal numbers
7 participants
@cgm616 @rust-highfive @ebroto @flip1995 @bors @rustbot @fhartwig