Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

strict vm shifts (2x) #167

Closed

Conversation

igormcoelho
Copy link
Contributor

Reducing limits and adding more tests for shifts.

@igormcoelho igormcoelho requested a review from shargon May 29, 2019 23:18
@codecov-io
Copy link

codecov-io commented May 29, 2019

Codecov Report

Merging #167 into master-2.x will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master-2.x     #167   +/-   ##
===========================================
  Coverage       59.03%   59.03%           
===========================================
  Files              17       17           
  Lines            1572     1572           
===========================================
  Hits              928      928           
  Misses            644      644
Impacted Files Coverage Δ
src/neo-vm/ExecutionEngine.cs 47.51% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15170c4...95d31c7. Read the comment docs.

@@ -22,7 +22,7 @@ public class ExecutionEngine : IDisposable
/// <summary>
/// Min value for SHL and SHR
/// </summary>
public virtual int Min_SHL_SHR => -256;
public virtual int Min_SHL_SHR => 0;
Copy link
Member

@shargon shargon May 29, 2019

Choose a reason for hiding this comment

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

We can change both variables to short, and we should check if is a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a breaking change, @vncoelho will make this sure for us ;)
short or int is the same here.

@shargon shargon changed the title strict vm shifts strict vm shifts (2x) May 29, 2019
@shargon
Copy link
Member

shargon commented May 29, 2019

we need to check the storages after this pr

@igormcoelho igormcoelho mentioned this pull request May 29, 2019
@erikzhang
Copy link
Member

Why we need this?

@erikzhang
Copy link
Member

static void Main(string[] args)
{
    BigInteger b0 = 1; //1
    BigInteger b1 = b0 << 1; //2
    BigInteger b2 = b1 << -1; //1
}

@erikzhang
Copy link
Member

This is BigInteger shift, not int shift.

Copy link
Member

@erikzhang erikzhang left a comment

Choose a reason for hiding this comment

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

Do not need.

@igormcoelho
Copy link
Contributor Author

Adding more tests here.

@igormcoelho
Copy link
Contributor Author

Correct Erik! Sorry for taking your time... some compiler situation tricked me. It converts automatically -1 on shift to 31... took some time to notice it. In other situations it seems to break compiling. I'll open an issue on neo-compiler project.

  EXEC : Convert error : System.Exception: error:System.Numerics.BigInteger Neo.SmartContract.HelloWorld::Main()::IL_0015 Call System.Numerics.BigInteger System.Numerics.BigInteger::op_LeftShift(System.Numerics.BigInteger,System.Int32) ---> System.Exception: unknown call: System.Numerics.BigInteger System.Numerics.BigInteger::op_LeftShift(System.Numerics.BigInteger,System.Int32) [/tmp/NeoContract1/NeoContract1.csproj]
  /tmp/NeoContract1/NeoContract1.csproj(66,5): error MSB3073: The command "./ConvertTask.sh" exited with code -1.

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.

None yet

4 participants