Skip to content
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 'isassigned' for RefValue #18082

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Add 'isassigned' for RefValue #18082

merged 1 commit into from
Jan 25, 2017

Conversation

malmaud
Copy link
Contributor

@malmaud malmaud commented Aug 17, 2016

Useful to avoid the need for users to break encapsulation by directly referencing the fields of RefValue

@tkelman tkelman added the needs tests Unit tests are required for this change label Aug 18, 2016
@stevengj
Copy link
Member

stevengj commented Aug 18, 2016

I would strongly prefer isdefined as in #18086, in analogy with Array.

@yuyichao
Copy link
Contributor

We already have isassigned why not just use it? isdefined as a builtin has a well defined meaning and shouldn't be overloaded.

@stevengj
Copy link
Member

@yuyichao, I'd rather get rid of isassigned entirely, as having both isdefined(array, i) and isassigned(array, i) with almost identical meanings seems odd and confusing to me.

The fact that isdefined is a builtin seems like an implementation detail. We can make Core.isdefined the builtin and have Base.isdefined be a generic-function wrapper.

@yuyichao
Copy link
Contributor

isdefined(array, i) is an implementation detail of isassigned(array, i) and will be gone after #12447 so isassigned(array, i) should always be used instead of isdefined(array, i).

More importantly for certain types (AbstractArrays for example) isdefined and isassigned are both meaningful and have non-compatible meanings so we can't have a single generic function wrapper for them and must have two functions (i.e. isdefined and isassigned).

@stevengj
Copy link
Member

In what case should isassigned(array, i) give a different result from isdefined(array, i)? I don't understand the problem you suggest with AbstractArray.

(There are plenty of cases where isdefined does not currently work because it is an intrinsic, but that is an implementation detail. The question is, why do we have two names for basically the same operation?)

@yuyichao
Copy link
Contributor

In what case should isassigned(array, i) give a different result from isdefined(array, i)

Yes. (Assuming you mean array::Array) The special case for array will be gone and isdefined(array, i) will return if the ith field of the new array object is defined or not. Note that this is already the case for some AbstractArray objects, which is the problem I suggested with AbstractArray.

julia> isdefined(view(Vector{Any}(3), :), 1)
true

julia> isassigned(view(Vector{Any}(3), :), 1)
false

The question is, why do we have two names for basically the same operation?

isdefined is used to determine if a field of an object is defined. isassigned is used to determine if a key of a container is defined.

@yuyichao
Copy link
Contributor

isdefined is used to determine if a field of an object is defined. isassigned is used to determine if a key of a container is defined.

i.e. they are not the same operation at all. It is unfortunate that isassigned for Array is implemented using the isdefined builtin, it would have been better if we use an arrayassigned intrinsic for that instead. Oh well.

@stevengj
Copy link
Member

stevengj commented Sep 5, 2016

Okay, looks like the consensus is for isassigned. Restarting Travis since the timeout error seems unrelated.

@kshyatt
Copy link
Contributor

kshyatt commented Sep 7, 2016

CI passed but looks like we still need tests?

@malmaud
Copy link
Contributor Author

malmaud commented Jan 19, 2017

@kshyatt Which test file does this belong in?

@malmaud
Copy link
Contributor Author

malmaud commented Jan 19, 2017

Tests added

@kshyatt
Copy link
Contributor

kshyatt commented Jan 19, 2017

Not sure, to be honest. Maybe whichever file(s) the ccall tests are in?

@malmaud
Copy link
Contributor Author

malmaud commented Jan 20, 2017

Good to merge?

@tkelman
Copy link
Contributor

tkelman commented Jan 20, 2017

Not worth creating a new test file and the printing and process spawning overhead of that for such a small nunber of new tests. ccall seems like a decent enough place for them, or misc

@tkelman tkelman removed the needs tests Unit tests are required for this change label Jan 20, 2017
@malmaud
Copy link
Contributor Author

malmaud commented Jan 22, 2017

@tkelman Done

@malmaud malmaud requested a review from tkelman January 25, 2017 16:31
@tkelman tkelman merged commit 0c6ff7a into master Jan 25, 2017
@tkelman tkelman deleted the jmm/ref_defined branch January 25, 2017 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants