-
-
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
Add Array{T,N}(nothing/missing, dims) convenience constructors #25054
Conversation
f4e79c1
to
27e138d
Compare
The new definitions cause weird redefinition warnings during bootstrap:
For some reason, they do not happen with the |
@@ -7,15 +7,19 @@ Core.AbstractArray | |||
Base.AbstractVector | |||
Base.AbstractMatrix | |||
Core.Array | |||
Core.Array(::Any) | |||
Core.Array(::Any, ::Any) | |||
Core.Array(::Uninitialized, ::Any) |
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.
Where these not included in the Array
entry on the line above?
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.
No, I got an error:
!! No docs found for 'Core.Array(::Any, ::Any)'. [src/stdlib/arrays.md]
I thought there was a way to include all methods, but this one doesn't work at least.
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.
Right, I meant that you should remove Core.Array(::Any)
and Core.Array(::Any, ::Any)
and keep Core.Array
. I think that Core.Array
should catch everything associated with Array
, but perhaps there was some reason for including them all earlier, perhaps to place the type docstring for Array
at the top?
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.
No, that only includes the type, not methods.
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.
Ok, good to know!
27e138d
to
f0e0342
Compare
I've found a way to avoid the "method redefined" warnings by defining the code in sysimg.jl together with existing |
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.
LGTM, lets give @Sacha0 the opportunity to review since he is the one engineering these type of array constructors :)
AppVeyor failure on 64-bit is the infamous Bunch-Kaufman bug. |
These are shorthands for fill!(Array{T,N}(uninitialized, dims...), nothing/missing). In the future they could be optimized for isbits Union types in which nothing/missing is the first type of the Union, since such arrays are already filled with nothing/missing.
f0e0342
to
dc6653c
Compare
These are shorthands for
fill!(Array{T,N}(uninitialized, dims), nothing/missing)
. In the future they could be optimized forisbits
Union
types in which nothing/missing is the first type of theUnion
, since such arrays are already filled withnothing
/missing
.This implements solution 3. from #24939, which got broad support.
I haven't found a way to attach the new docstrings to the new methods rather than to the general docstring for the function/type, given that methods are generated using
@eval
with a loop. Anyway the docstrings describe both thenothing
and themissing
method, so that sounds unavoidable to limit duplication.