add Type to kwargs internal argument #39593
Conversation
base/sysimg.jl
Outdated
| :Artifacts, | ||
| :ArgTools, |
There was a problem hiding this comment.
Out of curiosity, why reorder? (You successfully nerd sniped me 😄.)
There was a problem hiding this comment.
oops, that wasn't for this PR (I was timing ArgTools, and wondering why it was one of the slower packages to load)
|
Awesome, thanks so much! While you're at it, maybe change Line 293 in ce5bd1c to If you're at all worried about the possibility of invalidation regressions, you can check the invalidations from PyPlot. I'm sure you've done this before many times, perhaps using more low-level tools, but in case it saves you any time here's a recipe of how I'd do this: julia> using SnoopCompileCore
julia> invs = @snoopr using PyPlot;
julia> using SnoopCompile
julia> length(uinvalidated(invs))
1220😱 Something has already regressed! Looks like 8e61551. So the bar is low for this particular change, get it in before others raise the standards! 🙂 |
| Pairs{K, V}(data::A, itr::I) where {K, V, I, A} = Pairs{K, V, I, A}(data, itr) | ||
| Pairs{K}(data::A, itr::I) where {K, I, A} = Pairs{K, eltype(A), I, A}(data, itr) | ||
| Pairs(data::A, itr::I) where {I, A} = Pairs{eltype(I), eltype(A), I, A}(data, itr) | ||
| pairs(::Type{NamedTuple}) = Pairs{Symbol, V, NTuple{N, Symbol}, NamedTuple{names, T}} where {V, N, names, T<:NTuple{N, Any}} |
There was a problem hiding this comment.
This seems a bit strange to have as a method; maybe just make it a constant instead?
| sparams)) | ||
| (kw (gensy)) | ||
| (rkw (if (null? restkw) (make-ssavalue) (symbol (string (car restkw) "...")))) | ||
| (restkw (map (lambda (v) `(|::| ,v (call (top pairs) (core NamedTuple)))) restkw)) |
There was a problem hiding this comment.
This could be merged with the first definition of restkw.
There was a problem hiding this comment.
computing rkw needs the initial form (to stringify the variable name) before this wrapper is applied
That would be a dramatic change, since we allow constructing a Symbol from anything by stringification. |
|
Oh, funny I didn't know that. |
|
Have you come across a use case for it? I'm surprised to see the Invalidation Czar himself propose this :) |
|
Nah. For me Symbols live in their own world; until just now I also hadn't really that there are no objects in Base that convert to julia> methods(convert, (Type{Symbol}, Any))
# 1 method for generic function "convert":
[1] convert(::Type{T}, x::T) where T in Base at essentials.jl:171All this explains why that change passed tests and hasn't caused other known problems. I was just motivated by the fact that it looks like a generic method, even if nothing takes advantage of that. If nothing else we could document that the keys must be |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
vtjnash
removed
the
needs nanosoldier run
|
Thanks again, @vtjnash! |
since JuliaLang/julia#39593, this statement was not working anymore
since JuliaLang/julia#39593, this statement was not working anymore
Fixes #39419