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

Upgrade elastic array #5949

Merged
merged 2 commits into from
Jun 29, 2017

Conversation

eira-fransham
Copy link
Contributor

@eira-fransham eira-fransham commented Jun 28, 2017

This is a huge change, which includes some changes to replace code that originally cloned to reuse allocations instead. The updated elastic-array crate renames its consuming Vec-conversion method to into_vec, which means that I can do a simple sed -i 's/to_vec/into_vec/' and then fix the compilation errors.

This commit is probably a minor performance win and definitely a significant readability win.

@parity-cla-bot
Copy link

It looks like @Vurich signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

LGTM, 1 minor grumble

@@ -192,7 +192,7 @@ impl LightFetch {
let action = req.to.map_or(Action::Create, Action::Call);
let gas = req.gas.unwrap_or(U256::from(10_000_000)); // better gas amount?
let value = req.value.unwrap_or_else(U256::zero);
let data = req.data.map_or_else(Vec::new, |d| d.to_vec());
let data = req.data.unwrap_or(Vec::new());
Copy link
Collaborator

Choose a reason for hiding this comment

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

unwrap_or_default might be a better choice

@eira-fransham
Copy link
Contributor Author

I need to fix the build for tests (they don't get built by default so I only saw the problems when CI tried it out), I'll fix the minor grumble too (I 100% agree with it).

@rphmeier rphmeier added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. M4-core ⛓ Core client code / Rust. labels Jun 28, 2017
Vurich added 2 commits June 29, 2017 13:05
This is a huge change, which includes some changes to replace code that
originally cloned to reuse allocations instead. The updated
`elastic-array` crate renames its consuming `Vec`-conversion method to
`into_vec`, which means that I can do a simple
`sed -i 's/to_vec/into_vec/'` and then fix the compilation errors.

This commit is probably a minor performance win and definitely a
significant readability win.
@eira-fransham eira-fransham force-pushed the upgrade-elastic-array branch from 1db1050 to 01ce28b Compare June 29, 2017 13:12
@eira-fransham
Copy link
Contributor Author

Tests passing now, ready to merge?

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Jun 29, 2017
@rphmeier rphmeier merged commit a4195f2 into openethereum:master Jun 29, 2017
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.

4 participants