feat(clippy): add needless_pass_by_value

[tcoratger]

Should close #8886

[emhane]

I'm fine with this for the perf improvement, and imo the flexibility of having impl T instead of &impl T is rarely used, may break some public api though, so anywhere we should allow this @mattsse ?

[mattsse]

&impl looks very horrible

[mattsse]

this is very invasive and I don't want to let clippy decide how function signatures should look like.

I believe there are a few instances where the suggestions are valid, but I'm not a fan of this lint overall

[tcoratger]

this is very invasive and I don't want to let clippy decide how function signatures should look like.

I believe there are a few instances where the suggestions are valid, but I'm not a fan of this lint overall

@mattsse Tell me what you want to do, I have the impression that all the suggestions are more or less relevant and there is no false postive I have the impression.

From what I saw it removed unnecessary memory allocations due to cloning and made certain implementations more flexible in my opinion. I feel like this is a good guideline to follow for implementations when you're not sure whether to put a reference or a value.

On the other hand I noticed that it never altered the functioning of a function and no change other than passing by reference instead of by value was necessary.

For the &impl, I agree that it's horrible, if it's only in this place the problem, we can put an allow.

I also saw https://rust-lang.github.io/rust-clippy/master/#/needless_pass_by_ref_mut which can be a good addition to this to counterbalance.

But if there are too many places where this lint bothers you then it's not worth putting allows everywhere.

[emhane]

I even wanted to open an issue for fixing this is several places where we copy a 64 byte PeerId for now reason, where we can pass a ref. also I'm for the & impl because it helps informing the a type is sized! hope we can consider merging this rather sooner than later

[emhane]

& impl also gives guarantees ab the lifetime of the type, which is helpful

[Rjected]

I would rather deal with unnecessary pass by value w.r.t. clones on a case by case basis. While this can be helpful, it is still in the pedantic group of clippy lints. I personally prefer to have more control on APIs, since it sometimes makes sense to use pass by value

My preference would be to not add this lint, and instead only change the signature in places where we do have unnecessary clones or copies. This lint can be used locally anyways to catch these types of issues

[Rjected]

I would prefer this consume the keys for example

[tcoratger]

I would rather deal with unnecessary pass by value w.r.t. clones on a case by case basis. While this can be helpful, it is still in the pedantic group of clippy lints. I personally prefer to have more control on APIs, since it sometimes makes sense to use pass by value

My preference would be to not add this lint, and instead only change the signature in places where we do have unnecessary clones or copies. This lint can be used locally anyways to catch these types of issues

@Rjected I have the impression that if we do it on a case by case basis it will be difficult to maintain and very difficult for us to find the faulty elements, right?

Also I would say that if we apply the lint locally, this will slightly destructure our current classification which aims to apply the lints globally by registering them in the root cargo.toml right?

[emhane]

closing, @tcoratger feel free to recycle some of the code to change signatures in places identified as meaningful to pass by ref, for example

• PeerId, TxHash and maybe other Copy types bigger than usize as param, when not cloned in function body
• returning references in getters and cloning in place when needed, rather than cloning by default