-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: precompiled contract poc #172
feat: precompiled contract poc #172
Conversation
store/cachekv/store.go
Outdated
defer store.mtx.Unlock() | ||
|
||
// deep copy of cValue | ||
// TODO(dudong2): deep copy? or shallow copy? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Is there any other reason you comment TODO?
If type of val.value is slice of other reference type, it will be shallow copied but in this case, deep copy takes place.
I executed this code in golang playground and checked built-in copy function copies value of reference type variable.
a := []byte("abcd")
b := make([]byte, len(a))
copy(b, a)
a = []byte("1234")
fmt.Println(&a[0] == &b[0])
fmt.Println(a)
fmt.Println(b)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the copy of cValue needs to be a deep copy. I'd go with a deep copy first, and if there are performance issues, i'll need to devise another method.
store/cachekv/store.go
Outdated
|
||
// deep copy of cValue | ||
// TODO(dudong2): deep copy? or shallow copy? | ||
cache2 := make(map[string]*cValue, len(store.cache)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] It is chores but I suggest, we can use copiedCache, clonedCache instaed of cache2. This suggestion can be applied to other changes of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I had a few questions out of curiosity, so I left some comments
LGTM. [suggestion] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
2df2237
to
746c27b
Compare
rebased due to Send restriction PoC |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change