-
-
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
fix #23107, ensure Array
constructor always makes new arrays
#23490
Conversation
@nanosoldier |
6d904da
to
1670b52
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
We should have the same change applied to |
@@ -563,6 +563,12 @@ convert(::Type{Array{T,n}}, x::AbstractArray{S,n}) where {T,n,S} = copy!(Array{T | |||
|
|||
promote_rule(a::Type{Array{T,n}}, b::Type{Array{S,n}}) where {T,n,S} = el_same(promote_type(T,S), a, b) | |||
|
|||
# constructors should make copies |
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.
Is this documented somewhere? This is something I wondered when writing CategoricalArrays contructors. Since one-argument constructors fall back to convert
, by default they do not make copies. I agree it would make sense for convert
not to make a copy and for constructors to make a copy, but this should be stated clearly in the manual. Then we should review other types in Base (and in our packages) to check that this rule is followed consistently (not only for arrays).
EDIT: Sorry, I had missed the discussion at #23107. I agree with the conclusion there, so this is just a request to document this behavior instead of just silently doing 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.
1670b52
to
e9ab77d
Compare
I looked into the apparent memory increase in the |
There are possible remaining cases for |
Fix #23107.