Skip to content

Conversation

@Lunderberg
Copy link
Contributor

Provide a convenience method to update parameter struct info, propagating any changes to internal bindings and return type.

Provide a convenience method to update parameter struct info,
propagating any changes to internal bindings and return type.
Copy link
Contributor

@slyubomirsky slyubomirsky left a 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};
Copy link
Contributor

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).

Copy link
Contributor Author

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.

@Lunderberg
Copy link
Contributor Author

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?

That is correct. Only parameters would be updated, and variable bindings within the function body would have their struct info inferred.

I'm not sure if it would be simpler to override the function node case instead of VisitVarDef.

I initially ran into some issues with overriding the Expr VisitExpr_(const FunctionNode*) mutator, mostly because it was difficult without copy-pasting the entire ExprMutator implementation. In order to delegate to the base class's mutator, it would need to first construct a Function object with updated parameters, and then pass that to the base class. However, that ran into issues with the validity-checking in the Function constructor, since the body of the function wasn't yet updated.

Taking another look at it, it looks like it can be moved to the VisitExpr_(const FunctionNode*) by using func.CopyOnWrite() to avoid going through the normal constructor. I agree that this is simpler, and have updated the PR to use the less roundabout implementation.

Copy link
Contributor

@slyubomirsky slyubomirsky left a 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.

@Lunderberg Lunderberg merged commit d88cc42 into apache:unity Jan 4, 2024
@Lunderberg Lunderberg deleted the unity_transform_param_struct_info branch January 4, 2024 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants