Skip to content

Make copymutable public and add clarification to some docstrings #51939

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

Make copymutable public and add clarification to some docstrings #51939

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cfd187a
update sort docstring
Oct 30, 2023
50daaf5
make copymutable public
Oct 30, 2023
3b02a30
clarify that users should not typically need to override copymutable
Oct 30, 2023
fcf6313
Grammar
LilithHafner Oct 31, 2023
a03e2ef
xref similar
LilithHafner Oct 31, 2023
6647c1d
add to manual
Oct 31, 2023
d12cb60
clarify docstring and add StaticArrays example
Oct 31, 2023
aa09871
Merge branch 'master' into lh/sort-copymutable-doc
LilithHafner Nov 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions base/abstractarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,10 +1196,12 @@ end
"""
copymutable(a)

Make a mutable copy of an array or iterable `a`. For `a::Array`,
this is equivalent to `copy(a)`, but for other array types it may
differ depending on the type of `similar(a)`. For generic iterables
this is equivalent to `collect(a)`.
Make a mutable copy of an array or iterable `a`. The result should have the same content as
`a`, but support mutation. For `Array`s and most iterables this is equivalent to
`collect(a)`, but for other array types it may differ depending on the type of `similar(a)`.

To change the behavior of `copymutable` on a custom collection, define [`similar`](@ref)
for that collection.

# Examples
```jldoctest
Expand All @@ -1211,6 +1213,20 @@ julia> Base.copymutable(tup)
1
2
3

julia> using StaticArrays

julia> s = SVector{3}(1,2,3)
3-element SVector{3, Int64} with indices SOneTo(3):
1
2
3

julia> Base.copymutable(s)
3-element MVector{3, Int64} with indices SOneTo(3):
1
2
3
```
"""
function copymutable(a::AbstractArray)
Expand Down
1 change: 1 addition & 0 deletions base/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,7 @@ public
split_rest,
tail,
checked_length,
copymutable,

# Loading
DL_LOAD_PATH,
Expand Down
4 changes: 3 additions & 1 deletion base/sort.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,7 +1474,9 @@ end

Variant of [`sort!`](@ref) that returns a sorted copy of `v` leaving `v` itself unmodified.

Uses `Base.copymutable` to support immutable collections and iterables.
By default, `sort` copies its input with [`Base.copymutable`](@ref) before sorting, but some types have
specialized versions of `sort`, such as `NTuples` (`sort(::NTuple)` returns an `NTuple`) and
`Dict` (`sort(::Dict)` produces a new sorted dictionary data structure).
Copy link
Member

@rfourquet rfourquet Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I don't have a recent-enough julia version installed, but sort(::Dict) for me produces a vector of Pair and not a sorted dictionary, do you confirm this changed?

Copy link
Member

@rfourquet rfourquet Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related question, does sort(::Dict) returning a vector works thanks to the fact that copymutable is not yet specialized for Dict? Would implementing copymutable(d::Dict) = copy(d) break this? Would it make sense to support sort(Set(1)), and does it currently fail only because copymutable is used (instead of collect) for Set?

Copy link
Member

@rfourquet rfourquet Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, while copymutable seems fine for arrays, I wonder whether collect wouldn't be better than copymutable for other iterables. As copymutable for arrays is just a thin wrapper atop similar, mentioning "similar for arrays and collect for iterable" might be simpler than introducing another copymutable concept.

Copy link
Member Author

@LilithHafner LilithHafner Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, it's piracy by OrderedCollections that allows sort(::Dict) to work. OrderedCollections just gets loaded as a transitive dependency in my startup.jl file so I didn't realize. Also the pirated sort(::Dict) method is deprecated.

I don't like sort(::Dict) returning a vector of pairs, that is rarely what the user wants and it precludes the possibility of creating a better data structure instead.


!!! compat "Julia 1.10"
`sort` of arbitrary iterables requires at least Julia 1.10.
Expand Down
1 change: 1 addition & 0 deletions doc/src/base/collections.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ Base.tail
Base.step
Base.collect(::Any)
Base.collect(::Type, ::Any)
Base.copymutable(::Any)
Copy link
Member

@rfourquet rfourquet Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant to make copymutable public now, at least as long as it's not implemented for AbstractDict.

Base.filter
Base.filter!
Base.replace(::Any, ::Pair...)
Expand Down