-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Slightly generalize _compute_sparam elision #48144
Conversation
length(applyTbody.parameters) == length(arg.parameters) || return nothing | ||
for i = 1:length(applyTbody.parameters) | ||
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar | ||
return LiftedValue(argdef.args[3]) | ||
end | ||
end |
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 wonder if it it may be clearer (and more extensible later) if written this way:
length(applyTbody.parameters) == length(arg.parameters) || return nothing | |
for i = 1:length(applyTbody.parameters) | |
if applyTbody.parameters[i] === applyTvar && arg.parameters[i] === tvar | |
return LiftedValue(argdef.args[3]) | |
end | |
end | |
# find a possible parameter number we are extracting when matching with `tvar` | |
i = findfirst(===(tvar), arg.parameters) | |
# determine if we can find the apply_type for that parameter | |
if applyTvar === applyTbody.parameters[i] | |
return LiftedValue(argdef.args[3]) | |
end |
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'm not sure whether we want to look at all the parameters or just the first one. We can probably look at the first one in applyTbody, but I think arg
might potentially have multiple instances.
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.
First is indeed probably the most common to extract
To catch a case that occurs in FuncPipelines.jl and was causing precision issues in #48066.
isa(arg, DataType) || return nothing | ||
isType(arg) || return nothing |
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.
isa(arg, DataType) || return nothing | |
isType(arg) || return nothing | |
isType(arg) || return nothing |
isType(x)
includes isa(x, DataType)
.
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.
Will do in a follow up
I'd prefer not to. It's possible there further commits afterwards that are required |
To catch a case that occurs in FuncPipelines.jl and was causing precision issues in #48066.