Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Conditionally enable bitshift on ConstantinopleForkBlock #4838

Merged
merged 1 commit into from
Feb 22, 2018

Conversation

timxor
Copy link
Contributor

@timxor timxor commented Feb 8, 2018

Conditionally enable bitshift on ConstantinopleForkBlock

@timxor timxor changed the base branch from develop to bitshift February 8, 2018 00:35
@codecov-io
Copy link

codecov-io commented Feb 8, 2018

Codecov Report

Merging #4838 into bitshift will decrease coverage by 1.51%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##           bitshift    #4838      +/-   ##
============================================
- Coverage     60.67%   59.16%   -1.52%     
============================================
  Files           347      346       -1     
  Lines         27293    27249      -44     
  Branches       3151     2854     -297     
============================================
- Hits          16561    16121     -440     
- Misses         9738    10157     +419     
+ Partials        994      971      -23

@@ -33,6 +33,7 @@ struct EVMSchedule
bool haveDelegateCall = true;
bool eip150Mode = false;
bool eip158Mode = false;
bool eip145Mode = false;
Copy link
Member

Choose a reason for hiding this comment

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

We should better give it more descriptive name like haveBitwiseShifting

@gumb0
Copy link
Member

gumb0 commented Feb 21, 2018

Hey Tim, one more thing that needs to be done here is actually using the flag that you've added.

So the goal is to get rid of compile-time checks #if EIP_145 in #4054 and instead check this flag in every shift instruction

			if (!m_schedule->haveBitwiseShifting)
				throwBadInstruction();

// Pre-constantinople
if (!m_schedule->haveBitwiseShifting)
throwBadInstruction();

ON_OP();
updateIOGas();

Copy link
Member

Choose a reason for hiding this comment

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

There is a missed else-endif block below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Alex :)

@axic
Copy link
Member

axic commented Feb 22, 2018

Can you please squash some of these commits? :)

@timxor
Copy link
Contributor Author

timxor commented Feb 22, 2018

You got it, to do so makes me happy :D

@timxor
Copy link
Contributor Author

timxor commented Feb 22, 2018

Hoping this works ^_^

@axic axic merged commit 8c1291a into ethereum:bitshift Feb 22, 2018
@axic
Copy link
Member

axic commented Feb 22, 2018

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants