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: pretty-print remaining opcodes #1231

Merged
merged 7 commits into from
Aug 14, 2020
Merged

vm: pretty-print remaining opcodes #1231

merged 7 commits into from
Aug 14, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Jul 24, 2020

There are a few opcodes which have parameter but are not yet pretty-printed. Fix this.

@codecov
Copy link

codecov bot commented Jul 24, 2020

Codecov Report

Merging #1231 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1231      +/-   ##
==========================================
- Coverage   68.07%   68.05%   -0.02%     
==========================================
  Files         215      216       +1     
  Lines       18508    18524      +16     
==========================================
+ Hits        12599    12607       +8     
- Misses       5227     5235       +8     
  Partials      682      682              
Impacted Files Coverage Δ
pkg/core/interop/crypto/interop.go 100.00% <ø> (ø)
pkg/rpc/request/txBuilder.go 83.33% <0.00%> (ø)
pkg/vm/contract_checks.go 82.19% <ø> (ø)
pkg/vm/interop.go 78.26% <ø> (ø)
pkg/vm/vm.go 90.68% <25.00%> (-0.65%) ⬇️
pkg/compiler/codegen.go 89.67% <100.00%> (ø)
pkg/core/interop/callback/callback.go 100.00% <100.00%> (ø)
pkg/core/interop/context.go 96.72% <100.00%> (ø)
pkg/core/interop/interopnames/convert.go 100.00% <100.00%> (ø)
pkg/core/interops.go 100.00% <100.00%> (ø)
... and 5 more

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 95d86b6...b2e53fe. Read the comment docs.

@fyrchik
Copy link
Contributor Author

fyrchik commented Jul 25, 2020

Looks nice (from the test contract):

528      LDLOC          10 (0a)
530      CONVERT        Integer (21)
532      STLOC          9 (09)
534      LDLOC          9 (09)
536      LDLOC          8 (08)
538      LT
539      JMPIFNOT_L     575 (36/24000000)
544      PUSHDATA1      696e73756666696369656e742066756e6473 ("insufficient funds")
564      SYSCALL        System.Runtime.Log (cfe74796)

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

fyrchik commented Aug 13, 2020

I haven't implemented maps from id-to-name and reverse, because FromID is used only in PrintOps(). Not hard to add though.

@roman-khimov roman-khimov added this to the v0.91.0 milestone Aug 13, 2020
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.

neo-vm update (not strictly needed as the contents we care about didn't really change) should be explicit, not hidden in som a1b9706.

Have you tested 2136011 performance impact? It's being used in VM, so we shouldn't waste time there.

pkg/core/interop/names/convert.go Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
package names

var names = []string{
Copy link
Member

Choose a reason for hiding this comment

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

This is what I wanted to avoid --- yet another name registry duplicating declarations made by interops.

Copy link
Member

Choose a reason for hiding this comment

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

This is a perfect case for some code-generation tool, but Go isn't very friendly to those, so as a compromise I think we can make the following:

  • add interopnames (not names, please) package that would just contain string constants for all interop functions.
  • use its constants for interop registrations
  • use its constants for id-to-name in vm/cli

At least that would remove string duplication that is very easy to make error in. It still leaves some potential for constant usage to get out of sync, but that's unavoidable given the constraints we have.

@roman-khimov roman-khimov merged commit 40bcd4c into master Aug 14, 2020
@roman-khimov roman-khimov deleted the fix/printops branch August 14, 2020 11:43
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.

None yet

3 participants