-
Notifications
You must be signed in to change notification settings - Fork 442
refactor(gnovm): rename gnovm InterfaceType property Method to FieldTypes and allocator Allocate to Account #4823
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
base: master
Are you sure you want to change the base?
Conversation
Rename Allocator.Allocate to Allocator.Account to reflect that the method only performs memory accounting, gas billing, and GC-callback invocation — it does not perform Go runtime allocation. Update internal helper call sites to call Account(...) instead of Allocate(...).
Rename InterfaceType.Methods (type []FieldType) to FieldTypes to reflect that the slice can contain both methods and embedded interfaces, not only methods.
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
|
thank you for handling this. please see comment: #4794 (comment) |
| for i := range it.FieldTypes { | ||
| im := &it.FieldTypes[i] |
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.
seems in consistent with the func name.
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.
You right I missed this one, I think the function could be simply GetMethodFieldType => GetFieldType
What do you think ?
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.
As part of the same PR, do you think I should also rename methods to something else in:
gnovm/pkg/gnolang/nodes.go:704
type InterfaceTypeExpr struct {
Attributes
Methods FieldTypeExprs // list of methods
Generic Name // for uverse generics
}
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.
GetMethodFieldType => GetFieldType
looks good to me.
As part of the same PR, do you think I should also rename methods to something else in:
gnovm/pkg/gnolang/nodes.go:704
i think yes.
==========================================================
looking back to the InterfaceType, StructType, StructValue definition, i think there's some inaccurate in modification of Methods - > FieldTypes, perhaps Fields would be a better option—especially if we consider that Methods may not be entirely accurate due to embedding situation.
Overall, these are minor differences. I don’t have a strong opinion about changing the name due to the reason mentioned in the initial issue.
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 agree with the rename of Allocator.Allocate to Allocator.Account.
But I'm not convinced that InterfaceType.FieldTypes is better that InterfaceType.Methods. Even if objects implementing an interface have embedded structs to reach a particular method, interface types are still about methods only.
This reverts commit 55e90fa.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Addresses following issue: #4794
Rename Allocator.Allocate to Allocator.Account to reflect that the method only performs memory accounting — it does not perform Go runtime allocation.
Rename InterfaceType.Methods (type []FieldType) to FieldTypes to reflect that the slice can contain both methods and embedded interfaces, not only methods.