cpp: Extend API for creating results #333
Conversation
chfast
force-pushed
the
cpp_result
branch
2 times, most recently
from
fd7fffb
to
42c8d26
Compare
include/evmc/evmc.hpp
Outdated
| { | ||
| if (output_size != 0) | ||
| { | ||
| auto mem = static_cast<uint8_t*>(std::malloc(output_size)); |
There was a problem hiding this comment.
Apparently this can return nullptr. Why not use new and delete here instead?
There was a problem hiding this comment.
Not practical. I want to keep VM implementation low profile and require only C standard library if possible.
But good point raised. How about converting it to EVMC_INTERNAL_ERROR?
There was a problem hiding this comment.
How about converting it to EVMC_INTERNAL_ERROR?
I think that's a bad idea, because it masks what's going on. The caller would think the called contract failed.
There was a problem hiding this comment.
I'm not sure what do you mean by a caller here but in any case it should not take EVMC internal errors as indicators for contract execution failures.
I believe we only have 2 options here: ignore it or report it as an internal error.
chfast
force-pushed
the
cpp_result
branch
2 times, most recently
from
d55ce64
to
7184dd4
Compare
|
|
||
| if (!buffer) | ||
| { | ||
| result.status_code = EVMC_OUT_OF_MEMORY; |
There was a problem hiding this comment.
Still not convinced this it the right way to do it, given it confuses status of the execution vs. helpers, but if OOM is hit there is a bigger underlying issue. Perhaps worth highlighting this in the description above?
There was a problem hiding this comment.
but if OOM is hit there is a bigger underlying issue
What?
I don't thing this is the best way of passing output buffer. This will rather be only useful for Go, so we can drop this PR.
There was a problem hiding this comment.
but if OOM is hit there is a bigger underlying issue
What?
OOM = out of memory
Basically I'm ok with this change given this new specific error code is introduced.
There was a problem hiding this comment.
No, I had issue with understanding the sentence after "OOM". The OOM itself is difficult to handle in general. E.g. it will never happen on Linux in practice as it will happily try to allocate 2 TB of memory and in the end kill the process.
Anyway, the new specific error code was introduced: https://github.com/ethereum/evmc/pull/333/files#diff-8e9caa43721b131d9f7897e2bfd98299R309.
There was a problem hiding this comment.
Yeah I know, that's the line I commented on. Just having this documented on make_result would be useful.
|
I used this in Go: |
chfast
force-pushed
the
cpp_result
branch
2 times, most recently
from
d64965c
to
2d5cb58
Compare
This moves the evmc_result construction logic from evmc::result() constructor to helper.h. This function is now exposed as a C helper, C++ evmc::make_result() helper and used in the evmc::result() constructor. This is convenient for VM implementations which need to construct C evmc_result, not C++ evmc::result.
|
All fixed. |
Closes #258.
Usage check:
Test in Aleth (EvmCHost::call(),EvmCHost::create())