Avoid some clones using impl IntoIterator
#17831
Conversation
| .map_or_else(Vec::new, |filter| split_conjunction_owned(filter.clone())); | ||
| .map_or_else(Vec::new, |filter| split_conjunction(filter)); | ||
|
|
||
| let predicates: Vec<Expr> = predicates.into_iter().cloned().collect(); |
There was a problem hiding this comment.
Thanks @alamb
cloned() on iter calls a clone under the hood for each element?
https://doc.rust-lang.org/beta/std/iter/trait.Iterator.html#method.cloned
I'm trying to figure out the benefit
There was a problem hiding this comment.
The split_conjunction_owned -> split_conjunction change above avoids cloning the pred components.
But any benefit seems to be indeed voided by cloning here, which we execute unconditionally.
There was a problem hiding this comment.
Sorry I should have been clearer.
This code is splitting an expresision like
AND
AND
expr1
expr2
expr3
The cloned call still clones expr1, expr2, and expr3 but it doesn't clone the ANDs.
However, I agree that the benefit seems minor - I was more interested in alternate approaches than defining AsRef for Expr #17819
#17819 also included this improvement and I wanted to show it was also possible in another way too
I am happy to close this PR too
|
Let's go with #17819 |
3 participants
Which issue does this PR close?
AsRefforExpr#17819Rationale for this change
I feel like the
impl AsRef<T> for Tpattern isn't great, so I would like to try and avoid it unless there is no other wayFor example, @findepi has proposed adding it here
What changes are included in this PR?
Change signatures of functions that take
&[Expr](owned expressions) to take an iterator of&Exprwhich is what they needAre these changes tested?
By CI
Are there any user-facing changes?