-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[Unity][Transform] Implement UpdateParamStructInfo #16305
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
[Unity][Transform] Implement UpdateParamStructInfo #16305
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes.
Provide a convenience method to update parameter struct info, propagating any changes to internal bindings and return type.