[Unity][Transform] Implement UpdateParamStructInfo#16305
[Unity][Transform] Implement UpdateParamStructInfo#16305Lunderberg merged 4 commits intoapache:unityfrom
Conversation
Provide a convenience method to update parameter struct info, propagating any changes to internal bindings and return type.
Pull in bugfix from apache#16322
slyubomirsky
left a comment
There was a problem hiding this comment.
This seems reasonable, but I found it a little difficult to be certain of the logic. You are using VisitVarDef but are ignoring vars defined inside SeqExprs, so I take it that this will update only function parameters (per the name). Is that correct? It should perhaps be pointed out. I'm not sure if it would be simpler to override the function node case instead of VisitVarDef.
| } | ||
|
|
||
| TypedPackedFunc<Optional<StructInfo>(Var)> sinfo_func_; | ||
| bool inside_expr_{false}; |
There was a problem hiding this comment.
Nitpick: inside_expr_ might not be the most informative name, since this is checking for SeqExprs (or, from my understanding, function bodies in general).
There was a problem hiding this comment.
Good point, it probably would have been better to name it as inside_function_body_. It has been removed in the updated implementation.
That is correct. Only parameters would be updated, and variable bindings within the function body would have their struct info inferred.
I initially ran into some issues with overriding the Taking another look at it, it looks like it can be moved to the |
slyubomirsky
left a comment
There was a problem hiding this comment.
Thank you for the changes.
Provide a convenience method to update parameter struct info, propagating any changes to internal bindings and return type.