Replace &Option<T> with Option<&T> #5102
Conversation
github-actions
bot
added
core
| pub fn partition(&self) -> Option<&usize> { | ||
| self.partition.as_ref() | ||
| } |
There was a problem hiding this comment.
| pub fn partition(&self) -> Option<&usize> { | |
| self.partition.as_ref() | |
| } | |
| pub fn partition(&self) -> Option<usize> { | |
| self.partition.copied() | |
| } |
There was a problem hiding this comment.
I use .clone() instead of .copied() or .cloned(), because the compiler will raise error in last case:
error[E0599]: `Option<usize>` is not an iterator
--> datafusion\core\src\physical_plan\metrics\mod.rs:165:24
|
165 | self.partition.copied()
| ^^^^^^ `Option<usize>` is not an iterator
|
::: C:\Users\gaoxinge\.rustup\toolchains\nightly-x86_64-pc-windows-gnu\lib/rustlib/src/rust\library\core\src\option.rs:518:1
|
518 | pub enum Option<T> {
| ------------------ doesn't satisfy `Option<usize>: Iterator`
|
= note: the following trait bounds were not satisfied:
`Option<usize>: Iterator`
which is required by `&mut Option<usize>: Iterator`
This error may refer to Strange .cloned() error message mentioning iterators.
There was a problem hiding this comment.
Aah yes, got the various methods confused 👍
There was a problem hiding this comment.
I remove the .clone() to pass the ci, because it will cause the clippy error:
error: using `clone` on type `std::option::Option<usize>` which implements the `Copy` trait
--> datafusion/core/src/physical_plan/metrics/mod.rs:165:9
|
165 | self.partition.clone()
| ^^^^^^^^^^^^^^^^^^^^^^ help: try removing the `clone` call: `self.partition`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: `-D clippy::clone-on-copy` implied by `-D warnings`
|
Thank you |
|
Benchmark runs are scheduled for baseline = 3133526 and contender = 9c8bdfe. 9c8bdfe is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
This pr partly fix #4424.
Rationale for this change
#4424
What changes are included in this PR?
Replace
&Option<T>withOption<&T>.Are these changes tested?
Pass the original test.
Are there any user-facing changes?
May change the api.