Removed redundant struct bounds and unnecessary data copying #9096

debris · 2018-07-11T12:30:37Z

No description provided.

debris · 2018-07-11T12:31:06Z

ethcore/src/block.rs

@@ -488,11 +507,11 @@ impl<'x> IsBlock for OpenBlock<'x> {
 	fn block(&self) -> &ExecutedBlock { &self.block }
 }

-impl<'x> IsBlock for ClosedBlock {


redundant lifetime

debris · 2018-07-11T12:32:16Z

ethcore/src/client/client.rs

@@ -345,7 +344,7 @@ impl Importer {
 		imported
 	}

-	fn check_and_close_block(&self, block: PreverifiedBlock, client: &Client) -> Result<LockedBlock, ()> {
+	fn check_and_lock_block(&self, block: PreverifiedBlock, client: &Client) -> Result<LockedBlock, ()> {


renamed, cause the function returns LockedBlock, not ClosedBlock

debris · 2018-07-11T12:32:35Z

ethcore/src/client/client.rs

 		let hash = &header.hash();
 		let number = header.number();
 		let parent = header.parent_hash();
 		let chain = client.chain.read();

 		// Commit results
-		let receipts = block.receipts().to_owned();
-		let traces = block.traces().clone().drain();


receipts and traces were copied

debris · 2018-07-11T12:33:09Z

util/using_queue/src/lib.rs

@@ -19,7 +19,7 @@
 /// Special queue-like datastructure that includes the notion of
 /// usage to avoid items that were queued but never used from making it into
 /// the queue.
-pub struct UsingQueue<T> where T: Clone {


redundant struct bounds

dvdplm

Code lgtm, minor doc grumbles.

dvdplm · 2018-07-12T06:23:16Z

ethcore/src/block.rs

-//! Blockchain block.
+//! Base data structure of this module is `Block`.
+//!
+//! Blocks can be produced by a local node or they may be received from network.


"…the network."

dvdplm · 2018-07-12T06:23:51Z

ethcore/src/block.rs

+//! Blocks can be produced by a local node or they may be received from network.
+//!
+//! To create a block locally, we start with an `OpenBlock`. This block is mutable
+//! and can be appended with transactions and uncles.


"…can be appended to with transactions and uncles".

dvdplm · 2018-07-12T06:25:03Z

ethcore/src/block.rs

+//! To create a block locally, we start with an `OpenBlock`. This block is mutable
+//! and can be appended with transactions and uncles.
+//!
+//! When ready, `OpenBlock` can be closed and turned into `ClosedBlock`. `ClosedBlock` can


"…turned into a ClosedBlock. A ClosedBlock can…"

dvdplm · 2018-07-12T06:25:30Z

ethcore/src/block.rs

+//! and can be appended with transactions and uncles.
+//!
+//! When ready, `OpenBlock` can be closed and turned into `ClosedBlock`. `ClosedBlock` can
+//! be reopend again by miner under certain circumstances. On block close, state commit is


"…by a miner…"

dvdplm · 2018-07-12T06:27:02Z

ethcore/src/block.rs

 }

-/// Trait for a object that has a state database.
+/// Trait for a object that owns an `ExecutedBlock`


"…for an object…"

dvdplm · 2018-07-12T06:28:09Z

ethcore/src/block.rs

@@ -599,8 +617,8 @@ impl SealedBlock {

 impl Drain for SealedBlock {
 	/// Drop this object and return the underlieing database.


"…underlying database"

Also: is it true that it's a "database" being returned with this change?

it is! good catch!

dvdplm · 2018-07-12T06:39:22Z

ethcore/src/client/client.rs

-		let metadata = block.block().metadata().map(Into::into);
-		let is_finalized = block.block().is_finalized();
+		let metadata = block.metadata;
+		let is_finalized = block.is_finalized;


Do you really need to make new bindings like this? Maybe it's more readable to see block.is_finalized further down the code, makes it obvious where it's coming from if one reads only parts of the method?

fixed! Thanks for the review! :)

dvdplm

lgtm, and thanks for the added docs, most helpful!

niklasad1 · 2018-07-13T12:20:03Z

ethcore/src/block.rs

+//! Blocks can be produced by a local node or they may be received from the network.
+//!
+//! To create a block locally, we start with an `OpenBlock`. This block is mutable
+//! and can be appended to with transactions and uncles.


can be appended to with transactions and uncles.

No this is correct, "She appended an item to the list using a magnet" => "The list was appended to with a magnet"

niklasad1 · 2018-07-13T12:20:45Z

ethcore/src/block.rs

+//! and can be appended to with transactions and uncles.
+//!
+//! When ready, `OpenBlock` can be closed and turned into a `ClosedBlock`. A `ClosedBlock` can
+//! be reopend again by a miner under certain circumstances. On block close, state commit is


s/reopend/reopened

I will fix this typo in #9117

grbIzl

Excellent cleanup

Removed redundant struct bounds and unnecessary data copying

5422508

debris added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 11, 2018

debris commented Jul 11, 2018

View reviewed changes

5chdn added this to the 2.1 milestone Jul 11, 2018

debris commented Jul 11, 2018

View reviewed changes

dvdplm previously requested changes Jul 12, 2018

View reviewed changes

Updated docs, removed redundant bindings

65b7406

dvdplm approved these changes Jul 12, 2018

View reviewed changes

5chdn requested a review from grbIzl July 13, 2018 10:20

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 13, 2018

niklasad1 reviewed Jul 13, 2018

View reviewed changes

niklasad1 approved these changes Jul 13, 2018

View reviewed changes

niklasad1 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 Jul 13, 2018

grbIzl approved these changes Jul 13, 2018

View reviewed changes

debris merged commit f826ac3 into master Jul 15, 2018

debris deleted the block_cleanup branch July 15, 2018 09:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed redundant struct bounds and unnecessary data copying #9096

Removed redundant struct bounds and unnecessary data copying #9096

debris commented Jul 11, 2018

debris Jul 11, 2018

debris Jul 11, 2018

debris Jul 11, 2018

debris Jul 11, 2018

dvdplm left a comment

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm Jul 12, 2018

debris Jul 12, 2018

dvdplm Jul 12, 2018

dvdplm left a comment

niklasad1 Jul 13, 2018

dvdplm Jul 13, 2018

niklasad1 Jul 13, 2018

debris Jul 15, 2018

grbIzl left a comment

		@@ -599,8 +617,8 @@ impl SealedBlock {

		impl Drain for SealedBlock {
		/// Drop this object and return the underlieing database.

Removed redundant struct bounds and unnecessary data copying #9096

Removed redundant struct bounds and unnecessary data copying #9096

Conversation

debris commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grbIzl left a comment

Choose a reason for hiding this comment