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

Make the block header struct's internals private #2000

Merged
merged 3 commits into from
Aug 29, 2016

Conversation

nipunn1313
Copy link
Contributor

@nipunn1313 nipunn1313 commented Aug 24, 2016

Currently, this involves a lot of explicit cloning, but we
could migrate the return types of the get_* functions to
be copies rather than references since they are mostly copy
types anyway.

I opted to eliminate the constructor in favor of using
Default::default() plus calling a bunch of setters. This
is similar to the model that a Google Protobuf client uses
and I think it looks fine.

This begins work on #1267

@nipunn1313
Copy link
Contributor Author

Bam Pull Request number 2000!

@arkpar arkpar added the A8-looksgood 🦄 Pull request is reviewed well. label Aug 24, 2016

// TODO: consider removing these lines.
let min_difficulty = self.ethash_params.minimum_difficulty;
if header.difficulty < min_difficulty {
return Err(From::from(BlockError::DifficultyOutOfBounds(OutOfBounds { min: Some(min_difficulty), max: None, found: header.difficulty })))
if header.difficulty().clone() < min_difficulty {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be able to just do if header.difficulty() < &min_difficulty

@rphmeier rphmeier added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 25, 2016
@rphmeier
Copy link
Contributor

a lot of the clones in comparisons can be avoided but LLVM will probably optimize out extraneous copying. H256 probably shouldn't be Copy as it isn't that small imo.

Currently, this involves a lot of explicit cloning, but we
could migrate the return types of the get_* functions to
be copies rather than references since they are mostly copy
types anyway.

I opted to eliminate the constructor in favor of using
Default::default() plus calling a bunch of setters. This
is similar to the model that a Google Protobuf client uses
and I think it looks fine.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 86.366% when pulling 18c36f7 on nipunn1313:privateheader into 2d883c4 on ethcore:master.

@nipunn1313
Copy link
Contributor Author

Great point. I went through and found places where we could eliminate clone() by comparing references. Unfortunately, it seems like you can't compare a &H256 with an H256 directly because of rust-lang/rfcs#1332, but by making both sides references, it compiles nicely as @rphmeier suggested.

I also went through and fixed up the tests and tried to minimize explicit cloning. Any time + or * is used for math, I had to do some cloning. I think the llvm compiler will do a good job of optimizing those situations.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Aug 28, 2016
@arkpar arkpar merged commit 4389742 into openethereum:master Aug 29, 2016
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants