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

swap arguments for functions with more than 3 arguments #51

Merged
merged 1 commit into from
Oct 29, 2018
Merged

swap arguments for functions with more than 3 arguments #51

merged 1 commit into from
Oct 29, 2018

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Oct 26, 2018

Proposed changes in this pull request

Functions with more than 3 arguments are not working now.

func Main() {
	a := [4]int{0,0,0,0}
	a[0] = 0
	f1(a, 1, 2, 3)
}

func f1(a [4]int, x int, y int, z int) {
	a[1] = x
}

I think, I have fixed it with this pull request. What do you think?
make test still passes all tests

Type (put an x where ever applicable)

  • Bug fix: Link to the issue
  • Feature (Non-breaking change)
  • Feature (Breaking change)
  • Documentation Improvement

Checklist

Please put an x against the checkboxes. Write a small comment explaining if its N/A (not applicable)

  • Read the CONTRIBUTION guidelines.
  • All the tests are passing after the introduction of new changes.
  • Added tests respective to the part of code I have written.
  • Added proper documentation where ever applicable (in code and README.md).

Extra information

Any extra information related to this pull request.

Copy link
Collaborator

@anthdm anthdm left a comment

Choose a reason for hiding this comment

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

UPDATE: More then 3 needs some adustment. Sorry was looking at 3 arguments.

When looking in the neo-boa compiler I see the exact same thing in converting 3 functions arguments.

https://github.com/CityOfZion/neo-boa/blob/master/boa/code/vmtoken.py#L424

@anthdm
Copy link
Collaborator

anthdm commented Oct 26, 2018

This is working for me:

// Main in the contract entry point.
func Main() int {
    return someFunction(0, 1, 2, 3, 4, 5) // 6
}

func someFunction(a int, b int, c int, d int, e int, f int) int {
    // Make sure the compiler not flatten the arguments in a single result.
    va := a
    vb := b
    vc := c
    vd := d
    ve := e
    vf := f

    return va + vb + vc + vd + ve + vf
}

Testinvoke

{
  "state": "HALT, BREAK",
  "gas_consumed": "0.076",
  "script": "51c56b0051525354556506006c75665dc56b6a00527ac46a51527ac46a52527ac46a53527ac46a54527ac46a55527ac46a00c36a56527ac46a51c36a57527ac46a52c36a58527ac46a53c36a59527ac46a54c36a5a527ac46a55c36a5b527ac46a56c36a57c3936a58c3936a59c3936a5ac3936a5bc3936c7566",
  "Stack": [
    {
      "type": "Integer",
      "value": "15"
    }
  ]
}

@fyrchik
Copy link
Contributor Author

fyrchik commented Oct 26, 2018

@anthdm I think the way neo-boa does this is suboptimal (5 bytes per argument vs 2 in my PR if it is correct)
Regarding your example, it can behave good, because return value is symmetric in relation to function arguments. I was using [4]int with ints.

@anthdm
Copy link
Collaborator

anthdm commented Oct 26, 2018

@fyrchik If you test this with your fix, there is indeed no VM.FAULT, however, the stack is not looking too good.

@anthdm
Copy link
Collaborator

anthdm commented Oct 26, 2018

@fyrchik After testing your example with neo-boa I get the exact same result as with neo-storm after your fix. But like I said the stack is not cleaned up properly.

@anthdm
Copy link
Collaborator

anthdm commented Oct 26, 2018

@fyrchik UPDATE:

Your fix is correct after looking at the result for this example which only works after your fix.

func Main() string {
    c := f1(0, 1, 2, "foo")
    return c
}

func f1(a int, b int, c int, d string) string {
    return d
}
{
  "state": "HALT, BREAK",
  "gas_consumed": "0.03",
  "script": "52c56b00515203666f6f650e006a00527ac46a00c36c756655c56b6a00527ac46a51527ac46a52527ac46a53527ac46a53c36c7566",
  "Stack": [
    {
      "type": "ByteArray",
      "value": "666f6f"
    }
  ]
}

@fyrchik
Copy link
Contributor Author

fyrchik commented Oct 29, 2018

@anthdm could you please provide an example which leaves stack dirty? For me all your examples are working fine.

@anthdm anthdm merged commit 402ebb1 into CityOfZion:master Oct 29, 2018
roman-khimov pushed a commit to nspcc-dev/neo-go that referenced this pull request Aug 14, 2019
…eo-storm#51)

Imported from CityOfZion/neo-storm (402ebb1d6226e2a30d8fdc19663227361cc72ca0).
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.

2 participants