-
-
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
compiler: more accurate isdefined_tfunc
for NamedTuple
arg:
#37830
Conversation
c5bd1ee
to
3e33267
Compare
base/compiler/tfuncs.jl
Outdated
else | ||
ns = a1.parameters[1] | ||
if isa(ns, Tuple) | ||
return Const(val in ns) |
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.
So this happens when the NamedTuple is not concrete (e.g. because the types are not known), but the field names are known? Seems reasonable to me, but please add a test that tests this codepath (i.e. fails without this commit and passes with it).
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.
So this happens when the NamedTuple is not concrete (e.g. because the types are not known), but the field names are known?
yeah, exactly.
I will try to add a test later
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.
done
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 should use 1 <= idx <= length(ns)
.
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.
nice, applied in 8823625
fbf0f07
to
374b7f9
Compare
once JuliaLang/julia#37830 gets merged, this test should pass
374b7f9
to
8823625
Compare
once JuliaLang/julia#37830 gets merged, this test should pass
8823625
to
762a1dd
Compare
once JuliaLang/julia#37830 gets merged, this test should pass
e.g. this will exclude unnecessary runtime calls of `haskey` when a keyword argument is inferred as non-concrete type ```julia f(a; b = nothing, c = nothing) = return let b = rand((nothing,1,1.)) f(0; b) end ```
762a1dd
to
5464a9a
Compare
once JuliaLang/julia#37830 gets merged, this test should pass
once JuliaLang/julia#37830 gets merged, this test should pass
once JuliaLang/julia#37830 gets merged, this test should pass
once JuliaLang/julia#37830 gets merged, this test should pass
@JeffBezanson @Keno can you have a review on this again ? |
Nice improvement! |
once JuliaLang/julia#37830 gets merged, this test should pass
this will help the compiler, e.g. exclude unnecessary runtime calls of
haskey
when a keyword argument is inferred as non-concrete type