About warning: using `clone` on a `Copy` type [clone_on_copy]

Development
1

I’m fixing the warnings from clippy in pijul’s source code and i have a question:

In many places the types LineId, PatchId and Inode are explicitly cloned. These types are Copy types, and calling explicitly clone on it is unnecessary. They are copied by memcopy. But, i don’t know if is preferable for pijul, maybe by readibility reasons, or these types might not be more ‘copy’ types in the future.

Anyway, the question is: should I remove unnecessary calls to Clone for these types?

2 Likes
2

They were designed explicitly to be small types (possibly smaller than 64 bits), so you’re right, this is a design choice, and you can safely remove the clones.

I believe they come from the first version of Pijul, written in a language not low-level enough to provide guarantees on memory representation like "struct A([u8; 11], [u8; 31]) and struct B([u8; 42]) have the same representation".

(That language didn’t have [u8; N] to begin with).

1 Like
3

I was considering running clippy on Pijul as well, glad you’re doing it @gabomgp!

I have some related questions about code style:

  • There are a bunch of try! statements and that could directly be replaced by ?.
  • There are quite a few unnecessary return statements
  • In many cases an Ok(()) is returned at the bottom, where the Result of the previous statement could just as well be returned.

@pmeunier Would you accept patches that just “cleaned” these things up?

4

I would! These are usually unpleasant patches, because they are likely to cause conflicts. But go ahead! They are badly needed.

5

@pmeunier Thanks, will do!

@gabomgp I ran clippy myself and here’s the report. Some of the suggestions are quite interesting, like:

warning: this pattern creates a reference to a reference
  --> src/commands/keys.rs:92:17
   |
92 |     if let Some(ref user_host) = args.server {warning: this pattern creates a reference to a reference
  --> src/commands/keys.rs:92:17
   |
92 |     if let Some(ref user_host) = args.server {
   |                 ^^^^^^^^^^^^^
   |

   |                 ^^^^^^^^^^^^^
   |
   =    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#needless_borrow
1 Like