Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Light clippy(fy) #9473

Merged
merged 4 commits into from
Sep 6, 2018
Merged

Light clippy(fy) #9473

merged 4 commits into from
Sep 6, 2018

Conversation

niklasad1
Copy link
Collaborator

Ran clippy and fixed some issues!

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Sep 4, 2018
@@ -31,7 +31,7 @@ use ethereum_types::{H256, U256};
use memory_cache::MemoryLruCache;

/// Configuration for how much data to cache.
#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the reasons to add Copy trait here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is 5 usizes (max 40 bytes) and follows

Generally speaking, if your type can implement Copy, it should

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it ofc if you have other opinions!

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you update ethcore/res/wasm-tests submodule intentionally?

@niklasad1
Copy link
Collaborator Author

@ordian

Yes, I had some wasm-tests mismatches but I guess this is newer than master. Should I revert it to be on the safe side?

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had some wasm-tests mismatches but I guess this is newer than master. Should I revert it to be on the safe side?

Seems fine to me as long as all tests pass :)

@5chdn 5chdn added this to the 2.1 milestone Sep 5, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM just a minor question.

None => break,
while let Some(first) = self.sparse.pop_front() {
match self.complete_requests.remove(&first.hash()) {
None => { self.sparse.push_front(first); break; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-line maybe?


match self.complete_requests.remove(&start_hash) {
None => break,
while let Some(first) = self.sparse.pop_front() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know this code so I don't know if it makes sense to change this code to speculatively pop and then re-adding if complete_requests.remove fails. Is the success path the most common?

Copy link
Collaborator Author

@niklasad1 niklasad1 Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, Andre!

I don't know either!

This was more a hack to get around having a reference and mutating at the same time!
I somehow convinced myself by

This queue has O(1) amortized inserts and removals from both ends of the container

But now I realize that the Header struct is big (~600 bytes) so I will revert this thanks 🙌

@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 6, 2018
Copy link
Contributor

@lexfrl lexfrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@5chdn 5chdn merged commit 6888a96 into master Sep 6, 2018
@5chdn 5chdn deleted the light/clippyfy branch September 6, 2018 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants