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

Add byte.AsString and Map UTs #303

Merged
merged 18 commits into from
Jul 16, 2020

Conversation

ShawnYun
Copy link
Contributor

Close #297 .

  1. (5%)Compiler forget to convert byte[] in argument parameter list to Buffer
  2. (5%) AsString should convert Buffer to ByteString.
  3. (90%) Map UTs

@erikzhang erikzhang requested a review from lightszero June 23, 2020 07:23
lightszero
lightszero previously approved these changes Jun 23, 2020
@Tommo-L
Copy link
Contributor

Tommo-L commented Jun 24, 2020

Waiting for #302

@ShawnYun
Copy link
Contributor Author

ShawnYun commented Jul 1, 2020

Wait #317 .

@ShawnYun
Copy link
Contributor Author

ShawnYun commented Jul 2, 2020

Have fixed.

Comment on lines 126 to 129
if (method.paramtypes[pos].type.FullName == "System.Byte[]")
{
Insert1(VM.OpCode.CONVERT, "", to, new byte[] { (byte)VM.Types.StackItemType.Buffer });
}
Copy link
Member

Choose a reason for hiding this comment

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

Why should we do conversion when loading argument?

Copy link
Contributor Author

@ShawnYun ShawnYun Jul 2, 2020

Choose a reason for hiding this comment

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

Because a byte[] should be converted to Buffer.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. But you shouldn't convert it when loading. I think the conversion should be done when the data is pushed onto the stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here is the opcode LDARG, arg will be pushed onto stack.

Copy link
Member

Choose a reason for hiding this comment

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

The conversion should be done when the first time that the data is pushed onto the stack. LDARG loads the data from the slot, and the data from the slot is from stack before.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, you have to convert it everytime when load from slot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The args appears to be pushed on the stack before Loadscript? The compiler cannot convert it.

Copy link
Member

Choose a reason for hiding this comment

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

Then don't convert it. The argument should be string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can check the type of agrs in INITSLOT. Now in NeoVM we only check the count, not the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or we can check the type of agrs in INITSLOT.

I think it's good, it may not be efficient for compiler to check the parameter type after INITSLOT.

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.

Support byte[].AsString()
5 participants