Conversation
chfast
left a comment
There was a problem hiding this comment.
Try to omit std::unique_ptr. If any issues with this, let me know — I probably should fix it in EVMC then.
| evmc::VM* EVMHost::getVM(string const& _path) | ||
| { | ||
| static unique_ptr<evmc::vm> theVM; | ||
| static unique_ptr<evmc::VM> theVM; |
There was a problem hiding this comment.
The evmc::VM itself works as unique ptr, so it can be used directly here.
There was a problem hiding this comment.
Can you push a commit simplifying the entire function?
There was a problem hiding this comment.
Can you create a PR changing this after this PR is merged?
| { | ||
| evmc_loader_error_code errorCode = {}; | ||
| evmc_instance* vm = evmc_load_and_configure(_path.c_str(), &errorCode); | ||
| evmc_vm* vm = evmc_load_and_configure(_path.c_str(), &errorCode); |
There was a problem hiding this comment.
| evmc_vm* vm = evmc_load_and_configure(_path.c_str(), &errorCode); | |
| theVM = evmc::VM{evmc_load_and_configure(_path.c_str(), &errorCode)}; |
where theVM is of evmc::VM type.
There was a problem hiding this comment.
Didn't look at evmc and EVMHost (@chfast's comments are still outstanding there).
Apart from that and me wondering why there's no test expectation changes and some minor stuff, this looks good to me! Added links to the respective Istanbul EIPs (which are summarized in https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1679.md) for reference.
(and of course the fuzzing build needs fixing)
7be3141 to
a55d9a8
Compare
|
Rebased to see if we can somehow run the optimizer tests. |
e1f22c9 to
74c7d61
Compare
536f705 to
39a0abd
Compare
|
Need to update test expectations for the new gas prices. Otherwise I think it should be ready. |
|
Removed Istanbul changes from this PR. Let's merge this as is. |
bshastry
left a comment
There was a problem hiding this comment.
Could you please update the revision in the circleci README where it reads
The current revision is 1.
|
@bshastry done |
Should wait for evmc and evmone releases (potentially next week) before merging.
Part of #7310.
Closes #7544.