Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM: fix some bugs from JSON tests #394

Merged
merged 14 commits into from
Sep 16, 2019
Merged

VM: fix some bugs from JSON tests #394

merged 14 commits into from
Sep 16, 2019

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Sep 12, 2019

  • fail if NIP, (X)DROP, ROT or EQUAL have not enough arguments.
  • fail if XTUCK argument is 0
  • PICKITEM should accept bytearrays
  • NEWARRAY/NEWSTRUCT should create array of BoolItem{false} items instead of nils
  • REMOVE should return value of the same type as the argument (do not perform Struct -> Array conversion)
  • PUSH0 should push empty byte array (it is converted when 0 is needed)
  • NEWARRAY can accept []byte operand. Common usecase: PUSH0, NEWARRAY
  • SIZE can accept any argument castable to ByteArray
  • SHL/SHR do not change type of the operand if shift is by 0 (if applied to bytearray, leave bytearray as it is).

There are to more bugs specified in #196 . They include using Array as a pointer value and can me more prone to hidden bugs, so I will do this separately.

There are some tests in pkg/vm/tests/ which fail. I am not sure if tests or codegen should be fixed. E.g. if user is expecting Integer we could emit PUSH0, ADD on top of []byte item to return Integer. Or just make tests expect []byte{0}. All this implicit conversions stuff is rather annoying both from user's and developer's point of view.

pkg/vm/vm.go Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Show resolved Hide resolved
@roman-khimov
Copy link
Member

Looks like we have the same problem as in ca10fb0 with REVERSE.

pkg/vm/vm_test.go Show resolved Hide resolved
pkg/vm/vm_test.go Show resolved Hide resolved
pkg/vm/vm_test.go Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm.go Show resolved Hide resolved
pkg/vm/vm.go Outdated Show resolved Hide resolved
pkg/vm/vm_test.go Show resolved Hide resolved
@roman-khimov
Copy link
Member

There are some tests in pkg/vm/tests/ which fail

And BTW, we need to fix them one way or another before this merge.

@codecov
Copy link

codecov bot commented Sep 12, 2019

Codecov Report

Merging #394 into master will increase coverage by 0.43%.
The diff coverage is 78.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #394      +/-   ##
==========================================
+ Coverage   49.91%   50.34%   +0.43%     
==========================================
  Files          81       80       -1     
  Lines        5469     5430      -39     
==========================================
+ Hits         2730     2734       +4     
+ Misses       2484     2446      -38     
+ Partials      255      250       -5
Impacted Files Coverage Δ
pkg/vm/compiler/analysis.go 72.88% <100%> (ø) ⬆️
pkg/vm/stack.go 87.5% <100%> (+1.78%) ⬆️
pkg/vm/compiler/codegen.go 68.31% <40%> (ø) ⬆️
pkg/vm/vm.go 67.53% <84.61%> (+5.83%) ⬆️
pkg/core/storage/boltdb_store.go
pkg/core/storage/store.go 40.9% <0%> (+3.4%) ⬆️

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 b21a220...d2ccc3b. Read the comment docs.

pkg/vm/compiler/codegen.go Outdated Show resolved Hide resolved
pkg/vm/compiler/codegen.go Outdated Show resolved Hide resolved
pkg/vm/compiler/codegen.go Outdated Show resolved Hide resolved
@fyrchik
Copy link
Contributor Author

fyrchik commented Sep 12, 2019

There are some tests in pkg/vm/tests/ which fail

And BTW, we need to fix them one way or another before this merge.

I have added []byte{} in tests for now. In the future it is better to modify tests to support weak equality comparison or indeed change codegen.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

2ef8145 does more than it's written in the commit message, you should also mention changes made wrt type handling changes.

pkg/vm/vm_test.go Show resolved Hide resolved
@roman-khimov
Copy link
Member

46c56d1 should be squashed with c28c798, otherwise you have tests failing inbetween these commits.

@roman-khimov
Copy link
Member

Please, also fix REVERSE behavior to not push the result to the stack, it's the only one left with this bug after the current patchset.

@roman-khimov roman-khimov merged commit 0838948 into master Sep 16, 2019
@roman-khimov roman-khimov deleted the fix/json-tests-bugs branch September 16, 2019 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants