Update clippy and rustfmt

This patch
* prefers to use cmp
* makes code return early
* removes redundant cloning
* removes redundant lifetime annotation
* changes some other miscellaneous changes
to follow clippy update.

Author: Seulgi Kim

Diff for: .github/workflows/cargo-test.yml

@@ -10,7 +10,7 @@ jobs:
       - uses: actions/checkout@v1
       - uses: actions-rs/toolchain@v1
         with:
-            toolchain: nightly-2019-05-17
+            toolchain: nightly-2019-10-13
             override: true
       - run: rustup component add clippy
       - run: cargo fetch --verbose
@@ -23,7 +23,7 @@ jobs:
       - uses: actions/checkout@v1
       - uses: actions-rs/toolchain@v1
         with:
-            toolchain: nightly-2019-05-17
+            toolchain: nightly-2019-10-13
             override: true
       - run: rustup component add rustfmt
       - run: cargo fmt -- --check

Diff for: core/src/blockchain/extras.rs

@@ -150,7 +150,7 @@ impl Add for TransactionAddresses {
     type Output = Self;
 
     fn add(self, rhs: Self) -> <Self as Add>::Output {
-        let mut s = self.clone();
+        let mut s = self;
         s += rhs;
         s
     }
@@ -168,7 +168,7 @@ impl Sub for TransactionAddresses {
     type Output = Self;
 
     fn sub(self, rhs: Self) -> <Self as Add>::Output {
-        let mut s = self.clone();
+        let mut s = self;
         s -= rhs;
         s
     }

Diff for: core/src/blockchain/headerchain.rs

@@ -276,11 +276,11 @@ impl HeaderChain {
             };
             match route.retracted.len() {
                 0 => BestHeaderChanged::CanonChainAppended {
-                    best_header: new_best_header.clone(),
+                    best_header: new_best_header,
                 },
                 _ => BestHeaderChanged::BranchBecomingCanonChain {
                     tree_route: route,
-                    best_header: new_best_header.clone(),
+                    best_header: new_best_header,
                 },
             }
         } else {

Diff for: core/src/client/importer.rs

@@ -199,7 +199,7 @@ impl Importer {
         let mut batch = DBTransaction::new();
 
         block.state().journal_under(&mut batch, number).expect("DB commit failed");
-        let route = chain.insert_block(&mut batch, block_data, invoices.clone(), self.engine.borrow());
+        let route = chain.insert_block(&mut batch, block_data, invoices, self.engine.borrow());
 
         // Final commit to the DB
         client.db().write_buffered(batch);

Diff for: core/src/consensus/bit_set.rs

@@ -14,7 +14,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
-use std::cmp::PartialEq;
+use std::cmp::{Ordering, PartialEq};
 use std::convert::TryFrom;
 use std::fmt;
 use std::ops::Sub;
@@ -126,20 +126,20 @@ impl Decodable for BitSet {
         rlp.decoder().decode_value(|bytes| {
             let expected = BITSET_SIZE;
             let got = bytes.len();
-            if got > expected {
-                Err(DecoderError::RlpIsTooBig {
+            match got.cmp(&expected) {
+                Ordering::Greater => Err(DecoderError::RlpIsTooBig {
                     expected,
                     got,
-                })
-            } else if got < expected {
-                Err(DecoderError::RlpIsTooShort {
+                }),
+                Ordering::Less => Err(DecoderError::RlpIsTooShort {
                     expected,
                     got,
-                })
-            } else {
-                let mut bit_set = BitSet::new();
-                bit_set.0.copy_from_slice(bytes);
-                Ok(bit_set)
+                }),
+                Ordering::Equal => {
+                    let mut bit_set = BitSet::new();
+                    bit_set.0.copy_from_slice(bytes);
+                    Ok(bit_set)
+                }
             }
         })
     }

Diff for: core/src/consensus/stake/action_data.rs

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
+use std::cmp::Ordering;
 use std::collections::btree_map::{BTreeMap, Entry};
 use std::collections::btree_set::{self, BTreeSet};
 use std::collections::{btree_map, HashMap, HashSet};
@@ -203,12 +204,16 @@ impl<'a> Delegation<'a> {
         }
 
         if let Entry::Occupied(mut entry) = self.delegatees.entry(delegatee) {
-            if *entry.get() > quantity {
-                *entry.get_mut() -= quantity;
-                return Ok(())
-            } else if *entry.get() == quantity {
-                entry.remove();
-                return Ok(())
+            match entry.get().cmp(&quantity) {
+                Ordering::Greater => {
+                    *entry.get_mut() -= quantity;
+                    return Ok(())
+                }
+                Ordering::Equal => {
+                    entry.remove();
+                    return Ok(())
+                }
+                Ordering::Less => {}
             }
         }
 

Diff for: core/src/consensus/tendermint/worker.rs

@@ -14,6 +14,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
+use std::cmp::Ordering;
 use std::iter::Iterator;
 use std::mem;
 use std::sync::{Arc, Weak};
@@ -690,34 +691,33 @@ impl Worker {
                 cinfo!(ENGINE, "move_to_step: Propose.");
                 // If there are multiple proposals, use the first proposal.
                 if let Some(hash) = self.votes.get_block_hashes(&vote_step).first() {
-                    if self.client().block(&BlockId::Hash(*hash)).is_some() {
-                        self.proposal = Proposal::new_imported(*hash);
-                        self.move_to_step(TendermintState::Prevote, is_restoring);
-                    } else {
+                    if self.client().block(&BlockId::Hash(*hash)).is_none() {
                         cwarn!(ENGINE, "Proposal is received but not imported");
                         // Proposal is received but is not verified yet.
                         // Wait for verification.
                         return
                     }
-                } else {
-                    let parent_block_hash = self.prev_block_hash();
-                    if self.is_signer_proposer(&parent_block_hash) {
-                        if let TwoThirdsMajority::Lock(lock_view, locked_block_hash) = self.last_two_thirds_majority {
-                            cinfo!(ENGINE, "I am a proposer, I'll re-propose a locked block");
-                            match self.locked_proposal_block(lock_view, locked_block_hash) {
-                                Ok(block) => self.repropose_block(block),
-                                Err(error_msg) => cwarn!(ENGINE, "{}", error_msg),
-                            }
-                        } else {
-                            cinfo!(ENGINE, "I am a proposer, I'll create a block");
-                            self.update_sealing(parent_block_hash);
-                            self.step = TendermintState::ProposeWaitBlockGeneration {
-                                parent_hash: parent_block_hash,
-                            };
-                        }
-                    } else {
-                        self.request_proposal_to_any(vote_step.height, vote_step.view);
+                    self.proposal = Proposal::new_imported(*hash);
+                    self.move_to_step(TendermintState::Prevote, is_restoring);
+                    return
+                }
+                let parent_block_hash = self.prev_block_hash();
+                if !self.is_signer_proposer(&parent_block_hash) {
+                    self.request_proposal_to_any(vote_step.height, vote_step.view);
+                    return
+                }
+                if let TwoThirdsMajority::Lock(lock_view, locked_block_hash) = self.last_two_thirds_majority {
+                    cinfo!(ENGINE, "I am a proposer, I'll re-propose a locked block");
+                    match self.locked_proposal_block(lock_view, locked_block_hash) {
+                        Ok(block) => self.repropose_block(block),
+                        Err(error_msg) => cwarn!(ENGINE, "{}", error_msg),
                     }
+                } else {
+                    cinfo!(ENGINE, "I am a proposer, I'll create a block");
+                    self.update_sealing(parent_block_hash);
+                    self.step = TendermintState::ProposeWaitBlockGeneration {
+                        parent_hash: parent_block_hash,
+                    };
                 }
             }
             Step::Prevote => {
@@ -1562,7 +1562,7 @@ impl Worker {
             on,
         };
 
-        self.votes.collect(vote.clone()).expect("Must not attempt double vote on proposal");;
+        self.votes.collect(vote.clone()).expect("Must not attempt double vote on proposal");
         cinfo!(ENGINE, "Voted {:?} as {}th proposer.", vote, signer_index);
         Ok(vote)
     }
@@ -1811,7 +1811,7 @@ impl Worker {
                 );
             }
 
-            if let Err(double) = self.votes.collect(message.clone()) {
+            if let Err(double) = self.votes.collect(message) {
                 cerror!(ENGINE, "Double Vote found {:?}", double);
                 self.report_double_vote(&double);
                 return None
@@ -1869,17 +1869,15 @@ impl Worker {
 
         let current_step = current_vote_step.step;
         if current_step == Step::Prevote || current_step == Step::Precommit {
-            let peer_known_votes = if current_vote_step == peer_vote_step {
-                peer_known_votes
-            } else if current_vote_step < peer_vote_step {
+            let peer_known_votes = match current_vote_step.cmp(&peer_vote_step) {
+                Ordering::Equal => peer_known_votes,
                 // We don't know which votes peer has.
                 // However the peer knows more than 2/3 of votes.
                 // So request all votes.
-                BitSet::all_set()
-            } else {
+                Ordering::Less => BitSet::all_set(),
                 // If peer's state is less than my state,
                 // the peer does not know any useful votes.
-                BitSet::new()
+                Ordering::Greater => BitSet::new(),
             };
 
             let difference = &peer_known_votes - &self.votes_received;
@@ -1969,45 +1967,47 @@ impl Worker {
             return
         }
 
-        if height == self.height - 1 {
-            let block = self.client().block(&height.into()).expect("Parent block should exist");
-            let block_hash = block.hash();
-            let finalized_view = self.finalized_view_of_previous_block;
-
-            let votes = self
-                .votes
-                .get_all_votes_in_round(&VoteStep {
-                    height,
-                    view: finalized_view,
-                    step: Step::Precommit,
-                })
-                .into_iter()
-                .filter(|vote| vote.on.block_hash == Some(block_hash))
-                .collect();
-
-            self.send_commit(block, votes, &result);
-        } else if height < self.height - 1 {
-            let block = self.client().block(&height.into()).expect("Parent block should exist");
-            let child_block = self.client().block(&(height + 1).into()).expect("Parent block should exist");
-            let child_block_header_seal = child_block.header().seal();
-            let child_block_seal_view = TendermintSealView::new(&child_block_header_seal);
-            let parent_block_finalized_view =
-                child_block_seal_view.parent_block_finalized_view().expect("Verified block");
-            let on = VoteOn {
-                step: VoteStep::new(height, parent_block_finalized_view, Step::Precommit),
-                block_hash: Some(block.hash()),
-            };
-            let mut votes = Vec::new();
-            for (index, signature) in child_block_seal_view.signatures().expect("The block is verified") {
-                let message = ConsensusMessage {
-                    signature,
-                    signer_index: index,
-                    on: on.clone(),
+        match height.cmp(&(self.height - 1)) {
+            Ordering::Equal => {
+                let block = self.client().block(&height.into()).expect("Parent block should exist");
+                let block_hash = block.hash();
+                let finalized_view = self.finalized_view_of_previous_block;
+
+                let votes = self
+                    .votes
+                    .get_all_votes_in_round(&VoteStep {
+                        height,
+                        view: finalized_view,
+                        step: Step::Precommit,
+                    })
+                    .into_iter()
+                    .filter(|vote| vote.on.block_hash == Some(block_hash))
+                    .collect();
+
+                self.send_commit(block, votes, &result);
+            }
+            Ordering::Less => {
+                let block = self.client().block(&height.into()).expect("Parent block should exist");
+                let child_block = self.client().block(&(height + 1).into()).expect("Parent block should exist");
+                let child_block_header_seal = child_block.header().seal();
+                let child_block_seal_view = TendermintSealView::new(&child_block_header_seal);
+                let parent_block_finalized_view =
+                    child_block_seal_view.parent_block_finalized_view().expect("Verified block");
+                let on = VoteOn {
+                    step: VoteStep::new(height, parent_block_finalized_view, Step::Precommit),
+                    block_hash: Some(block.hash()),
                 };
-                votes.push(message);
+                let mut votes = Vec::new();
+                for (index, signature) in child_block_seal_view.signatures().expect("The block is verified") {
+                    let message = ConsensusMessage {
+                        signature,
+                        signer_index: index,
+                        on: on.clone(),
+                    };
+                    votes.push(message);
+                }
             }
-
-            self.send_commit(block, votes, &result);
+            Ordering::Greater => {}
         }
     }
 
@@ -2049,23 +2049,27 @@ impl Worker {
             return None
         }
 
-        if commit_height < self.height {
-            cdebug!(
-                ENGINE,
-                "Received commit message is old. Current height is {} but commit messages is for height {}",
-                self.height,
-                commit_height,
-            );
-            return None
-        } else if commit_height > self.height {
-            cwarn!(
-                ENGINE,
-                "Invalid commit message received: precommit on height {} but current height is {}",
-                commit_height,
-                self.height
-            );
-            return None
-        }
+        match commit_height.cmp(&self.height) {
+            Ordering::Less => {
+                cdebug!(
+                    ENGINE,
+                    "Received commit message is old. Current height is {} but commit messages is for height {}",
+                    self.height,
+                    commit_height,
+                );
+                return None
+            }
+            Ordering::Greater => {
+                cwarn!(
+                    ENGINE,
+                    "Invalid commit message received: precommit on height {} but current height is {}",
+                    commit_height,
+                    self.height
+                );
+                return None
+            }
+            Ordering::Equal => {}
+        };
 
         let prev_block_hash = self
             .client()

Diff for: core/src/miner/mem_pool.rs

@@ -960,7 +960,7 @@ impl MemPool {
 
     /// Returns Some(true) if the given transaction is local and None for not found.
     pub fn is_local_transaction(&self, tx_hash: H256) -> Option<bool> {
-        self.by_hash.get(&tx_hash).and_then(|found_item| Some(found_item.origin.is_local()))
+        self.by_hash.get(&tx_hash).map(|found_item| found_item.origin.is_local())
     }
 
     /// Checks the given timelock with the current time/timestamp.
@@ -1479,7 +1479,7 @@ pub mod test {
         inputs.push(create_mempool_input_with_pay(7u64, keypair, no_timelock));
         mem_pool.add(inputs, inserted_block_number, inserted_timestamp, &fetch_account);
 
-        let mut mem_pool_recovered = MemPool::with_limits(8192, usize::max_value(), 3, db.clone());
+        let mut mem_pool_recovered = MemPool::with_limits(8192, usize::max_value(), 3, db);
         mem_pool_recovered.recover_from_db(&test_client);
 
         assert_eq!(mem_pool_recovered.first_seqs, mem_pool.first_seqs);
@@ -1549,7 +1549,7 @@ pub mod test {
         let test_client = TestBlockChainClient::new();
 
         let db = Arc::new(kvdb_memorydb::create(crate::db::NUM_COLUMNS.unwrap_or(0)));
-        let mut mem_pool = MemPool::with_limits(8192, usize::max_value(), 3, db.clone());
+        let mut mem_pool = MemPool::with_limits(8192, usize::max_value(), 3, db);
 
         let fetch_account = |p: &Public| -> AccountDetails {
             let address = public_to_address(p);

Diff for: keystore/src/accounts_dir/disk.rs

@@ -329,7 +329,7 @@ mod test {
         directory.insert_with_filename(account.clone(), "foo".to_string(), dedup).unwrap();
         let file1 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap();
         let file2 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap();
-        let file3 = directory.insert_with_filename(account.clone(), filename.clone(), dedup).unwrap().filename.unwrap();
+        let file3 = directory.insert_with_filename(account, filename.clone(), dedup).unwrap().filename.unwrap();
 
         // then
         // the first file should have the original names