-
Notifications
You must be signed in to change notification settings - Fork 383
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
fix(gnovm): follow up works on loop scope(#2429) #2440
fix(gnovm): follow up works on loop scope(#2429) #2440
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev/jae/loopescape #2440 +/- ##
===================================================
Coverage 54.79% 54.79%
===================================================
Files 583 583
Lines 78965 78929 -36
===================================================
- Hits 43269 43250 -19
+ Misses 32464 32450 -14
+ Partials 3232 3229 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
241ecc9
to
c6411d6
Compare
…well/jae/loopscope
…well/jae/loopscope
…well/jae/loopscope
…well/jae/loopscope
for _, rte := range n.Type.Results { | ||
if rte.Name != "" { | ||
n.Predefine(false, rte.Name) | ||
for i, rte := range n.Type.Results { | ||
if rte.Name == "" { | ||
rn := fmt.Sprintf(".res_%d", i) | ||
rte.Name = Name(rn) | ||
} | ||
n.Predefine(false, rte.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.
define hidden .res_x in initStaticBlocks
to preserve the original init order: [param_0, param_1...param_n, res_0, res_1...res_n
, var_0, var_1...var_n] . the order is changed by he initStaticBlocks logic to be [param_0, param_1...param_n , var_0, var_1...var_n, res_0, res_1...res_n
].
This order will impact defer logic.
Another workaround is to modify the runtime returnToBlock and returnFromBlock logic.
// }, | ||
// "FileName": "main.gno", | ||
// "IsMethod": false, | ||
// "Name": "init.0", | ||
// "Name": "init.1", |
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.
is this introduced by the initStaticBlocks
?
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.
yes.
// "@type": "/gno.PointerType", | ||
// "Elt": { | ||
// "@type": "/gno.RefType", | ||
// "ID": "gno.land/p/demo/avl.Node" | ||
// } | ||
// }, | ||
// "V": { | ||
// "@type": "/gno.PointerValue", | ||
// "Base": { | ||
// "@type": "/gno.RefValue", | ||
// "Hash": "e925bb6bf884ece9620dd27f831dc510b5bc0e55", | ||
// "ObjectID": "a8ada09dee16d791fd406d629fe29bb0ed084a30:6" | ||
// }, | ||
// "Index": "0", | ||
// "TV": null | ||
// } | ||
// }, | ||
// { | ||
// "T": { | ||
// "@type": "/gno.FuncType", |
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.
ditto
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.
package var is inited firstly in initStaticBlocks
, the the order changes.
@@ -4311,7 +4366,7 @@ func setNodeLocations(pkgPath string, fileName string, n Node) { | |||
// XXX check node lines, uniqueness of locations, | |||
// and also check location pkgpath and filename. | |||
func checkNodeLinesLocations(pkgPath string, fileName string, n Node) { | |||
// XXX | |||
// TODO: XXX |
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.
should be in another PR.
n.SetAttribute(ATTR_LOOP_DEFINES, lus) | ||
// XXX implement delete. | ||
// n.DelAttribute(ATTR_LOOP_USES) | ||
// no need anymore | ||
n.DelAttribute(ATTR_LOOP_USES) | ||
n.DelAttribute(ATTR_LOOP_DEFINES) | ||
} |
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.
no need anymore?
last.Predefine(false, n.Key.(*NameExpr).Name) | ||
nx := n.Key.(*NameExpr) | ||
if nx.Name != blankIdentifier { | ||
nx.Type = NameExprTypeDefine | ||
last.Predefine(false, nx.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.
maybe we should not support loopvar with range either, to make it consistent with for loop.
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.
yes. but TypeDefine is correct here as it is a define.
@@ -2549,28 +2569,43 @@ func addHeapCapture(dbn BlockNode, fle *FuncLitExpr, name Name) uint16 { | |||
ne := NameExpr{ | |||
Path: vp, | |||
Name: name, | |||
Type: NameExprTypeHeapClosure, | |||
Type: NameExprTypeHeapClosure, // XXX, this is actually not used, remove? |
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.
right it's not actually used, but let's leave it because it is a special kind of use of a name, to copy it to func value captures. It just means it was captured by a closure, for now.
|
||
// Copy *FuncValue.Captures into block | ||
// NOTE: addHeapCapture in preprocess ensures order. | ||
// XXX, actually 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.
I think we can remove the XXX right?
Brilliant. |
e.g.
this is supported,
this is not supported: