Skip to content

chore: apply clippy suggestions newly introduced in rust 1.86 #7382

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
Merged
westonpace merged 1 commit into from
Apr 3, 2025
Merged

chore: apply clippy suggestions newly introduced in rust 1.86 #7382

westonpace merged 1 commit into from
Apr 3, 2025

Conversation

@westonpace
Copy link
Member

@westonpace westonpace commented Apr 3, 2025 •
edited by alamb
Loading

Which issue does this PR close?

Rationale for this change

See issue

What changes are included in this PR?

Use div_ceil (available since rustc 1.73, over 1 year ago)
Use is_empty instead of len() == 0

Are there any user-facing changes?

No

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels Apr 3, 2025
@westonpace
Copy link
Member Author

westonpace commented Apr 3, 2025 •
edited
Loading

There is some discussion of div_ceil in #7358 which suggests there should not be a performance hit.

That being said, that discussion was centered around the value / divisor + (0 != value % divisor) formulation which is slightly different than the value + (divisor-1) / divisor formulations which were getting caught here. No idea if there is a significant difference. Maybe @mbutrovich knows more?

etseidl
etseidl approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@etseidl etseidl left a comment

Choose a reason for hiding this comment

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

+1 Thanks @westonpace. I was just about to run down this hole to fix CI when I noticed this PR 😅.

@mbutrovich
Copy link
Contributor

mbutrovich commented Apr 3, 2025

I am expecting equivalent machine code due to the operands, but I'll confirm within 30 minutes.

alamb
alamb approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @westonpace

I'll wait for @mbutrovich to confirm but then I think we should merge this one in

mbutrovich
mbutrovich approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@mbutrovich mbutrovich left a comment

Choose a reason for hiding this comment

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

https://www.diffchecker.com/Kb0zO8go/ the relevant bits are around line 167

The original implementation which is just an add and shift right immediate 6 is gonna be more efficient than the stdlib implementation that includes some conditional increments and conditional sets. That function is written to handle arbitrary operands rather than hardcoded arithmetic, so I'm not sure you'd be able to wrangle equivalent machine code out of the stdlib. It's probably not worth worrying about too much.

@westonpace westonpace merged commit 43ee8cf into apache:main Apr 3, 2025
29 checks passed
@alamb
Copy link
Contributor

alamb commented Apr 3, 2025

Thanks for checking @mbutrovich

@alamb alamb mentioned this pull request Apr 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@etseidl etseidl etseidl approved these changes

+1 more reviewer

@mbutrovich mbutrovich mbutrovich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arrow Changes to the arrow crate parquet Changes to the parquet crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New clippy failures in code base with release of rustc 1.86

4 participants

@westonpace @mbutrovich @alamb @etseidl