-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
switch OpaqueClosure from isva
field to separate argument tuple type
#44008
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
Conversation
1ee5507
to
4380028
Compare
cd09901
to
59089f0
Compare
5bccf4b
to
de69e0d
Compare
Instead allow specifying an argument tuple type separately.
de69e0d
to
709d73d
Compare
Instead allow specifying an argument tuple type separately.
Instead allow specifying an argument tuple type separately.
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 cleanup. I think there is a missing JL_TYPECHK / jl_is_datatype though (at the marked place)?
// TODO: remove | ||
jl_value_t *jl_fptr_va_opaque_closure(jl_opaque_closure_t *oc, jl_value_t **args, size_t nargs) | ||
// determine whether `argt` is a valid argument type tuple for the given opaque closure method | ||
JL_DLLEXPORT int jl_is_valid_oc_argtype(jl_tupletype_t *argt, jl_method_t *source) |
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 to assume that argt
is a datatype, without checking?
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.
jl_new_opaque_closure
checks jl_is_tuple_type
, and the other caller in codegen does as well.
Instead allow specifying an argument tuple type separately.
Depends on #43320, but I will put this up now for review.
TODO: Add signature type compatibility check in codegen.