-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Conversation
#include "VMConfig.h" | ||
#include "VMFace.h" | ||
|
||
#include <libevm/VMFace.h> |
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.
So libaleth-interpreter depends on libevm? This is going to change later, right?
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.
Only on headers. This should change, but is not a priority. Home helper functions are defined in VMFace.h. I didn't want to do a lot of changes when moving files.
At least this PR allow building aleth-interpreter as a shared library and distribute it in this form.
libaleth-interpreter/VMConfig.h
Outdated
@@ -0,0 +1,433 @@ | |||
/* |
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.
Hm, why does github doesn't show this file as moved
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.
Did you change 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.
Ah is it copied?
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.
It's a copy. The config was shared between legacy and interpreter VMs.
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.
Perhaps you want to move the file to libaleth-interpreter
and place a copy into the original directory?
That way if libaleth-interpreter
is split out of the main tree using "git magic" the history will stay proper.
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 don't think VMConfig is super important, but I can try to do it.
I believe there are 2 easy changes that can be done here later. Use instruction metrics from evmc library and do not include headers from libevm. Maybe third would be to remove some compile time optimization options (only leave the default values). But we have to decide what to do with #5065 as these 2 are in conflict. |
The above is my only comment and I like the change FWIW. Would vote to merge this first, the other PR should detect if files are moved during a rebase and apply the changes properly. |
Codecov Report
@@ Coverage Diff @@
## develop #5080 +/- ##
===========================================
- Coverage 60.95% 58.62% -2.33%
===========================================
Files 337 335 -2
Lines 26787 26434 -353
Branches 3162 3136 -26
===========================================
- Hits 16327 15496 -831
- Misses 9404 9932 +528
+ Partials 1056 1006 -50 |
977db3b
to
270a346
Compare
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.
Applied requested git history changes. GitHub might be confused because the commits are not in chronological order.
@@ -0,0 +1,444 @@ | |||
/* |
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 a copy now.
No description provided.