This repository has been archived by the owner on Nov 22, 2023. It is now read-only.
Allow to convert a Struct from Array #91
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ixje
reviewed
Mar 24, 2019
src/neo-vm/ExecutionEngine.cs
Outdated
|
|
||
| for (var i = 0; i < array.Count; i++) | ||
| { | ||
| items.Add(array[i]); |
There was a problem hiding this comment.
Given that Struct subclasses Array, I would think the fastest option could be to change the pointer of
neo-vm/src/neo-vm/Types/Array.cs
Line 10 in c552114
to the equivalent of the new
Struct object. The more items the more speed gain. I just don't know if that's possible with C#. Maybe with the unsafe keyword? It just feels bad seeing code looping over items to (almost) only change the type of the class. Just dropping ideas, no C# expert here.
erikzhang
approved these changes
Mar 25, 2019
5 tasks
bors bot
added a commit
to neo-one-suite/neo-one
that referenced
this pull request
Mar 28, 2019
1256: refactor(PACK+NEWSTRUCT): Replace PACKSTRUCT with PACK+NEWSTRUCT r=dicarlo2 a=afragapane ### Description of the Change Removes the PACKSTRUCT Opcode and modifies NEWSTRUCT/NEWARRAY to be able to convert an existing array into a struct and struct into array. This allows the use of a PACK+NEWSTRUCT pair of opcodes in place of PACKSTRUCT. ### Test Plan Added tests for the conversions in NEWSTRUCT and NEWARRAY. Updated existing tests to reflect the slightly higher GAS costs of neo-one compiled contracts. ### Alternate Designs Previously the plan was to implement our PACKSTRUCT in the neo-vm as per neo-project/neo-vm#51 and neo-project/neo-vm#88. This new solution was suggested and implemented here: neo-project/neo-vm#91. ### Benefits ### Possible Drawbacks Very slightly higher GAS cost for neo-one compiled smart contracts. ### Applicable Issues #1254 Co-authored-by: afragapane <ahfragapane@gmail.com>
bors bot
added a commit
to neo-one-suite/neo-one
that referenced
this pull request
Mar 28, 2019
1256: refactor(PACK+NEWSTRUCT): Replace PACKSTRUCT with PACK+NEWSTRUCT r=dicarlo2 a=afragapane ### Description of the Change Removes the PACKSTRUCT Opcode and modifies NEWSTRUCT/NEWARRAY to be able to convert an existing array into a struct and struct into array. This allows the use of a PACK+NEWSTRUCT pair of opcodes in place of PACKSTRUCT. ### Test Plan Added tests for the conversions in NEWSTRUCT and NEWARRAY. Updated existing tests to reflect the slightly higher GAS costs of neo-one compiled contracts. ### Alternate Designs Previously the plan was to implement our PACKSTRUCT in the neo-vm as per neo-project/neo-vm#51 and neo-project/neo-vm#88. This new solution was suggested and implemented here: neo-project/neo-vm#91. ### Benefits ### Possible Drawbacks Very slightly higher GAS cost for neo-one compiled smart contracts. ### Applicable Issues #1254 Co-authored-by: afragapane <ahfragapane@gmail.com>
This was referenced Sep 6, 2019
shargon
pushed a commit
that referenced
this pull request
Sep 9, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #51
Closes #88
Allow to convert from Array to Struct and from Struct to array using
NEWARRAYandNEWSTRUCTOpcodes