-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
I think Travis is failing because the C++ compiler/standard library is rather old, there doesn't seem to be anything wrong with that piece of code. |
include/evm2wasm.h
Outdated
|
||
// maps the async ops to their call back function | ||
static std::map<opcodeEnum, std::string> callbackFuncs = {{opcodeEnum::SSTORE, "$callback"}, | ||
{opcodeEnum::SLOAD, "$callback_256"}, {opcodeEnum::CREATE, "$callback_160"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not include more of the on the same line, just one per line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ea8ff91
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axic You have to give up hand-tuning the code formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This format is super hard to read, there must be a way to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found a trick to do it in clang-format: add ,
after the last item.
Is |
bin/evm2wasm.js
Outdated
@@ -1,4 +1,4 @@ | |||
#!/usr/bin/env node | |||
#!/usr/bin/env nodejs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
std::string opcodeToString(opcodeEnum opcode) | ||
{ | ||
switch (opcode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a map for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid callers of this function to deal with std::pair
, but I can change it to a map if you prefer.
Just a bit of
Yeah, I agree, it's rather awful 😄 I'm going to split them up and see if they look better. |
I'm not sure how much work it is, but I'd rather stick to C++ library only. Adding additional dependencies is quite a big deal and we also want to compile that to WASM later on. If possible I'd use just |
@esteve do you want to create a new PR for the samples? |
Also can you please rebase and squash some commits? |
The samples are mine, I can pull these out into a separate PR, good idea
…On 26 March 2018 at 19:51, Alex Beregszaszi ***@***.***> wrote:
Also can you please rebase and squash some commits?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#201 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADKbNGfcHAcjUlB-kjHaEVjFbgmQtVtoks5tiX7qgaJpZM4S3JV7>
.
|
But go for it if you get to it first, @esteve!
…On 26 March 2018 at 20:02, Lane Rettig ***@***.***> wrote:
The samples are mine, I can pull these out into a separate PR, good idea
On 26 March 2018 at 19:51, Alex Beregszaszi ***@***.***>
wrote:
> Also can you please rebase and squash some commits?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#201 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ADKbNGfcHAcjUlB-kjHaEVjFbgmQtVtoks5tiX7qgaJpZM4S3JV7>
> .
>
|
e519924
to
ea70359
Compare
@lrettig can you rebase this so it runs on circleci? Also the first and last commit are not useful anymore (the useful parts have been merged since). |
ea70359
to
3c49c5c
Compare
@axic done, sorry for the delay. Been busy lately, but I'll address the pending feedback in the upcoming days (I hope :-)) |
92fb153
to
4ed84b7
Compare
4ed84b7
to
fe693a9
Compare
include/evm2wasm.h
Outdated
@@ -351,9 +351,9 @@ struct WastCode | |||
std::string imports; | |||
}; | |||
|
|||
std::string evm2wasm(const std::string& input); | |||
std::string evm2wasm(const std::vector<char>& input); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change the public API? And what is the desired content after the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bad choice of the type. If you want to handle multiple input types consider using string_view
(not in standard yet) or pass iterators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points guys. Maybe it wasn't the most well-thought-out change on my part, and I will revert it.. My thinking was that bytecode is better expressed as an array of bytes as opposed to a null-terminated string.
750793d
to
3db366d
Compare
32721ae
to
d9fba6a
Compare
… initialization list in C++
…execution in tests
143eaf3
to
a01efa5
Compare
a01efa5
to
a4fc405
Compare
This is just a quick and rather hacky C++ port of the Javascript translator. I've added a dependency on Hunter and
fmtlib
for the string manipulation code and also copied over the.clang-format
from cpp-ethereum.Connects to #4