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

two small changes in evm #5700

Merged
merged 3 commits into from
May 30, 2017
Merged

two small changes in evm #5700

merged 3 commits into from
May 30, 2017

Conversation

guanqun
Copy link
Contributor

@guanqun guanqun commented May 27, 2017

  • add one unit test for is_push in instructions.rs
  • split VMType into a separate file as suggested by the TODO in factory.rs (no functional changes)

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm pretty sure you need to update EVM tests and EVMbin cause it's also using VMType.

@guanqun
Copy link
Contributor Author

guanqun commented May 27, 2017

evmbin doesn't seem to use VMType, I did a quick grep:

➜  parity $ cd evmbin
➜  evmbin $ git grep VMType

The main two folders that are using VMType are ethcore and parity

$ git grep VMType | tr "/" " " | awk '{ print $1}' | sort | uniq
ethcore
parity

And these two components can cargo build and cargo test properly. So I think it's fine.

Code-wise, in evm/mod.rs, I'm pulling the VMType directly into evm module, and that's the same behavior as before. As long as other code is using evm::VMType instead of evm::factory::VMType, then we're fine. :)

EDIT: grep how we use the module:

git grep "use.*evm::.*VMType"
ethcore/src/client/config.rs:pub use evm::VMType;
ethcore/src/client/evm_test_client.rs:use evm::{self, VMType};
ethcore/src/client/test_client.rs:use evm::{Factory as EvmFactory, VMType, Schedule};
ethcore/src/evm/benches/mod.rs:use evm::{self, Factory, VMType};
ethcore/src/evm/tests.rs:use evm::{self, Ext, Schedule, Factory, GasLeft, VMType, ContractCreateResult, MessageCallResult, CreateContractAddress};
ethcore/src/executive.rs:       use evm::{Factory, VMType, CreateContractAddress};
ethcore/src/json_tests/executive.rs:use evm::{Schedule, Ext, Factory, Finalize, VMType, ContractCreateResult, MessageCallResult, CreateContractAddress};

Looks like we don't need to change the downstream code.

@guanqun
Copy link
Contributor Author

guanqun commented May 28, 2017

Do we have other external repo that depends on this?

BTW: How can I trigger an integration build for this PR?

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels May 28, 2017
@@ -19,6 +19,7 @@
pub mod ext;
pub mod evm;
pub mod interpreter;
mod vmtype;
Copy link
Contributor

@rphmeier rphmeier May 29, 2017

Choose a reason for hiding this comment

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

style: separate public modules from private modules with whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, moved down a bit.

use std::fmt;

#[derive(Debug, PartialEq, Clone)]
/// Type of EVM to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

style: docs over attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 30, 2017
@NikVolf
Copy link
Contributor

NikVolf commented May 30, 2017

#5718
for the CI check

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels May 30, 2017
@gavofyork gavofyork merged commit 5a20c63 into openethereum:master May 30, 2017
@gavofyork
Copy link
Contributor

谢谢!

@guanqun guanqun deleted the read-evm branch May 31, 2017 02:21
@guanqun
Copy link
Contributor Author

guanqun commented May 31, 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.

5 participants