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

Change unusual_byte_groupings to require byte groupings of equal size #10353

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

nindalf
Copy link
Contributor

@nindalf nindalf commented Feb 16, 2023

Fixes issue #6556

This lint required byte groupings of size 2 or 4 for Radix::Binary and Radix::Hexadecimal. Since there are good reasons for allowing groups of other sizes, this PR relaxes the restriction. This lint now requires that

  • group sizes after the first group be of the same size and
  • greater or equal in size to the first group.

changelog: [unusual_byte_groupings]: reduce false positives by relaxing restriction requiring groups of specific sizes.

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Alexendoo (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2023
@@ -210,7 +210,7 @@ impl WarningType {
cx,
UNUSUAL_BYTE_GROUPINGS,
span,
"digits of hex or binary literal not grouped by four",
"digits of hex, binary or octal literal not groups of equal size",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"digits of hex, binary or octal literal not groups of equal size",
"digits of hex, binary or octal literal not in groups of equal size",

@@ -427,8 +427,12 @@ impl LiteralDigitGrouping {

let first = groups.next().expect("At least one group");

if (radix == Radix::Binary || radix == Radix::Hexadecimal) && groups.any(|i| i != 4 && i != 2) {
return Err(WarningType::UnusualByteGroupings);
if radix == Radix::Binary || radix == Radix::Octal || radix == Radix::Hexadecimal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if radix == Radix::Binary || radix == Radix::Octal || radix == Radix::Hexadecimal {
if radix != Radix::Decimal {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this keeps the intention clearer.

@nindalf nindalf marked this pull request as ready for review February 16, 2023 11:29
@nindalf
Copy link
Contributor Author

nindalf commented Feb 16, 2023

r? @llogiq

@rustbot rustbot assigned llogiq and unassigned Alexendoo Feb 16, 2023
@llogiq
Copy link
Contributor

llogiq commented Feb 16, 2023

Thank you! @bors r+

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

📌 Commit f12b492 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

⌛ Testing commit f12b492 with merge 86fb33a...

@bors
Copy link
Collaborator

bors commented Feb 16, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 86fb33a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants