RFC: make Ref(x) always construct a RefValue(x) object #21527
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ararslan
reviewed
Apr 24, 2017
base/refpointer.jl
Outdated
| (::Type{Ref{T}})() where {T} = RefValue{T}() # Ref{T}() | ||
| (::Type{Ref{T}})(x) where {T} = RefValue{T}(x) # Ref{T}(x) | ||
| convert(::Type{Ref{T}}, x) where {T} = RefValue{T}(x) | ||
|
|
||
| Ref(x::Ref, i::Integer) = (i != 1 && error("Ref only has one element"); x) |
There was a problem hiding this comment.
throw(ArgumentError()) would probably be better than error() here.
|
Somewhat related #18990 |
ararslan
added
needs docs
|
Way too late to be adding new deprecations to 0.6. |
|
Yes, this absolutely cannot be merged until we branch. |
tkelman
reviewed
May 31, 2017
base/deprecated.jl
Outdated
| @@ -1328,6 +1328,10 @@ for fname in (:ones, :zeros) | |||
| end | |||
| end | |||
|
|
|||
| @deprecate Ref(x::AbstractArray) Ref(x, 1) | |||
There was a problem hiding this comment.
should be moved to 0.7 section
|
LGTM. I added a 1.0 tag. Just needs a rebase & merge. |
vtjnash
removed
needs docs
the old behavior is now directly a feature of `convert`
vtjnash
force-pushed
the
jn/Ref-always
branch
from
03388bf
to
c6b56d3
Compare
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
|
Sorry, wrong reference commit makes this a bit confusing to read the report. But shows that the performance issue observed in the nightly occurred before this PR was merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After having
Ref(x)on master for awhile, it seems awkward to need to writeRef{typeof(x)}(x)to be sure to get a reference toxitself, especially sincexmay often be a Ptr. This reservesRef(x)to mean precisely "make a Ref containingx". Further, it extends two common usages (Ref{<:Number}()andRef{<:Ptr}()) to always be zero-initialized so thatRef{Ptr{T}}()works.(the old behavior of only making a Ref if the argument was not already a subtype of Ref is now directly a feature of
convert)