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

Remove unnecessary clones with clippy #12197

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Remove unnecessary clones with clippy #12197

merged 3 commits into from
Aug 28, 2024

Conversation

findepi
Copy link
Member

@findepi findepi commented Aug 27, 2024 •
edited
Loading

See individual commit descriptions.

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate execution Related to the execution crate proto Related to proto crate functions labels Aug 27, 2024
@findepi
Remove unnecessary clones with clippy
3724a74 
This is automated change done with

```
cargo clippy --fix -- -Aclippy::all -Wclippy::redundant_clone
cargo fmt
 # manually fix few new clippy errors introduced by clippy itself
```

Note: it doesn't remove all unnecessary clones because the command
reported error and backed out for the common crate.
@findepi findepi force-pushed the findepi/remove-unnecessary-clones-with-clippy-e8ec70 branch from d1c824f to 3724a74 Compare August 27, 2024 14:51
@findepi
Prevent clippy from removing intentional clone
782de5f 
clippy can be run with `--fix` and then it won't obey the code comment
instructing not to delete the clone.
@findepi
Remove unnecessary clones pointed out by clippy
af7594c 
Change code as instructed by

```
cargo clippy --fix -- -Aclippy::all -Wclippy::redundant_clone
```

where clippy didn't apply the suggested changes by itself.
@github-actions github-actions bot added the common Related to common crate label Aug 27, 2024
@findepi findepi mentioned this pull request Aug 27, 2024
Merged
@findepi
Copy link
Member Author

findepi commented Aug 27, 2024

Note 1:
clippy doesn't seem to find all unnecessary clones (#12196 (comment))

Note 2
Running cargo clippy -- -Aclippy::all -Wclippy::needless_pass_by_value reports quite many functions which today move arguments but could take reference. Addressing these could potentially lead to some more cloning becoming unnecessary (and fixable with clippy). However clippy's --fix doesn't seem to work with needless_pass_by_value for me, this may require manual work. Not sure this is worth it, maybe the compiler can figure it all out on its own?

@crepererum crepererum merged commit 8ba6732 into apache:main Aug 28, 2024
24 checks passed
@findepi findepi deleted the findepi/remove-unnecessary-clones-with-clippy-e8ec70 branch August 28, 2024 07:49
@findepi
Copy link
Member Author

findepi commented Aug 28, 2024

thank you @crepererum and @andygrove for review and thanks @crepererum for the merge!

BTW by any chance, do you have thoughts on needless_pass_by_value mentioned above?

@crepererum
Copy link
Contributor

BTW by any chance, do you have thoughts on needless_pass_by_value mentioned above?

I think we can tighten the linting rules for DF. In this case, add them here and fix the fall-out in the same PR:

datafusion/Cargo.toml

Lines 160 to 162 in 8ba6732

[workspace.lints.clippy]
# Detects large stack-allocated futures that may cause stack overflow crashes (see threshold in clippy.toml)
large_futures = "warn"

@findepi findepi mentioned this pull request Aug 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crepererum crepererum crepererum approved these changes

@andygrove andygrove andygrove approved these changes

Assignees
No one assigned
Labels
common Related to common crate core Core DataFusion crate execution Related to the execution crate functions logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Physical Expressions proto Related to proto crate sql SQL Planner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@findepi @crepererum @andygrove