`in` uses `isequal` (#9381) #24563

JackDevine · 2017-11-10T03:48:17Z

I changed the docstring quite a bit, I think that it is now a good description of the current behavior. I also added the test in the first comment of #9381.

Edit: fix #9381

I changed the docstring quite a bit, I think that it is now a good description of the current behavior. I also added the test in the first comment of #9381.

StefanKarpinski · 2017-11-10T04:15:19Z

There are quite a few more methods of in – did you check all of them?

JackDevine · 2017-11-10T04:40:43Z

Thanks for pointing that out, I was just about to ask about that. For example, in associative.jl

function in(p::Pair, a::Associative, valcmp=(==))
    v = get(a,p[1],secret_table_token)
    if v !== secret_table_token
        valcmp(v, p[2]) && return true
    end
    return false
end

Do you want valcmp to become isequal by default? My understanding was that we are happy with how in works for dictionaries and sets. Also, would

in(x::Char, y::Char) = x == y

goes to

in(x::Char, y::Char) = isequal(x, y)

change anything?

When I look into this more, there are definitely a few that I need to change e.g. in(x::Number, y::Number). Should I only make changes where they might make a difference or should I just be greedy and make lots of changes?

StefanKarpinski · 2017-11-10T05:08:46Z

Yes, I would change all of these. In some cases == behaves the same as isequal but if someone were to override or change the behavior of isequal but not == then we want this to be consistent everywhere. Thanks for tackling this!

ararslan · 2017-11-10T07:37:23Z

NEWS.md

@@ -272,6 +272,8 @@ This section lists changes that do not have deprecation warnings.
 Library improvements
 --------------------

+  * `in` now uses `isequal` to test for containment ([#9381]).


This should probably be listed under breaking changes instead.

Good point, fixed.

andyferris · 2017-11-12T04:00:25Z

I also wonder if == for a container should be isequal for all the elements. For example, [NaN] == [NaN] - well, they are the same container, no?. Like NaN, this is also important for null/missing.

JackDevine · 2017-11-12T04:44:42Z

Is that idea relevant to this PR? I don't think that it is inconsistent to have in use isequal while still having [NaN] !== [NaN].

andyferris · 2017-11-12T11:31:42Z

Is that idea relevant to this PR?

Not in the sense that this PR needs modifying. :)

However, I think there is a fair amount of relevance to #9381 (actually I should have probably put my comment there instead, sorry!) in that I would say that containers would be equal when they contain (i.e. element in container) the same elements. Obviously NaN in [NaN] is relevant to this PR, but we also have !([NaN] == [NaN]) but Set(NaN) == Set(NaN) which is exactly the kind of silliness that is being addressed here.

JeffBezanson · 2017-11-14T17:22:30Z

base/associative.jl

@@ -67,7 +67,7 @@ function next(v::ValueIterator, state)
    n[1][2], n[2]
 end

-in(k, v::KeyIterator) = get(v.dict, k, secret_table_token) !== secret_table_token
+in(k, v::KeyIterator) = !isequal(get(v.dict, k, secret_table_token), secret_table_token)


This is not quite right. This is using !== (which is the opposite of ===, not ==) to see if exactly the secret_table_token object was returned.

JeffBezanson · 2017-11-14T17:24:51Z

base/env.jl

@@ -4,7 +4,7 @@ if Sys.iswindows()
    const ERROR_ENVVAR_NOT_FOUND = UInt32(203)

    _getenvlen(var::Vector{UInt16}) = ccall(:GetEnvironmentVariableW,stdcall,UInt32,(Ptr{UInt16},Ptr{UInt16},UInt32),var,C_NULL,0)
-    _hasenv(s::Vector{UInt16}) = _getenvlen(s) != 0 || Libc.GetLastError() != ERROR_ENVVAR_NOT_FOUND
+    _hasenv(s::Vector{UInt16}) = !isequal(_getenvlen(s), 0) || !isequal(Libc.GetLastError(), ERROR_ENVVAR_NOT_FOUND)


These don't need to change.

JeffBezanson · 2017-11-14T17:25:07Z

base/env.jl

@@ -38,7 +38,7 @@ if Sys.iswindows()
    end
 else # !windows
    _getenv(var::AbstractString) = ccall(:getenv, Cstring, (Cstring,), var)
-    _hasenv(s::AbstractString) = _getenv(s) != C_NULL
+    _hasenv(s::AbstractString) = !isequal(_getenv(s), C_NULL)


JeffBezanson · 2017-11-14T17:26:43Z

base/show.jl

@@ -87,7 +87,7 @@ pipe_writer(io::IOContext) = io.io
 lock(io::IOContext) = lock(io.io)
 unlock(io::IOContext) = unlock(io.io)

-in(key_value::Pair, io::IOContext) = in(key_value, io.dict, ===)
+in(key_value::Pair, io::IOContext) = in(key_value, io.dict, isequal)


This should not change since the caller was specifically requesting ===.

JeffBezanson · 2017-11-14T17:28:02Z

base/strings/search.jl

@@ -44,7 +44,7 @@ function search(s::AbstractString, c::Chars, i::Integer)
 end
 search(s::AbstractString, c::Chars) = search(s,c,start(s))

-in(c::Char, s::AbstractString) = (search(s,c)!=0)
+in(c::Char, s::AbstractString) = !isequal(search(s,c), 0)


This should also not change, since it isn't comparing c; it's testing the result of a different function.

JackDevine · 2017-11-14T22:00:50Z

Thanks @JeffBezanson , I may have gone a bit overboard with some of those changes. I have changed back to the old style in all of the places that you asked.

JackDevine · 2018-01-04T06:12:57Z

Closing due to comment in #9381

ararslan · 2018-01-04T06:34:18Z

Thank you regardless for your efforts here, @JackDevine!

in uses isequal (#9381)

359c2f7

I changed the docstring quite a bit, I think that it is now a good description of the current behavior. I also added the test in the first comment of #9381.

fredrikekre added breaking This change will break code collections Data structures holding multiple items, e.g. sets labels Nov 10, 2017

ararslan reviewed Nov 10, 2017

View reviewed changes

Change is breaking and applied to more methods

87160ce

JackDevine mentioned this pull request Nov 12, 2017

should in use isequal to test for containment? #9381

Closed

JeffBezanson requested changes Nov 14, 2017

View reviewed changes

JeffBezanson reviewed Nov 14, 2017

View reviewed changes

Correct the usage of isequal in some places

5626ddc

mauro3 mentioned this pull request Dec 9, 2017

== for immutables should recursively call == on its fields #4648

Open

JackDevine closed this Jan 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`in` uses `isequal` (#9381) #24563

`in` uses `isequal` (#9381) #24563

JackDevine commented Nov 10, 2017 •

edited by fredrikekre

Loading

StefanKarpinski commented Nov 10, 2017

JackDevine commented Nov 10, 2017

StefanKarpinski commented Nov 10, 2017 •

edited

Loading

ararslan Nov 10, 2017

JackDevine Nov 11, 2017

andyferris commented Nov 12, 2017

JackDevine commented Nov 12, 2017

andyferris commented Nov 12, 2017

JeffBezanson Nov 14, 2017

JeffBezanson Nov 14, 2017

JeffBezanson Nov 14, 2017

JeffBezanson Nov 14, 2017

JeffBezanson Nov 14, 2017

JackDevine commented Nov 14, 2017

JackDevine commented Jan 4, 2018

ararslan commented Jan 4, 2018

in uses isequal (#9381) #24563

in uses isequal (#9381) #24563

Conversation

JackDevine commented Nov 10, 2017 • edited by fredrikekre Loading

StefanKarpinski commented Nov 10, 2017

JackDevine commented Nov 10, 2017

StefanKarpinski commented Nov 10, 2017 • edited Loading

ararslan Nov 10, 2017

Choose a reason for hiding this comment

JackDevine Nov 11, 2017

Choose a reason for hiding this comment

andyferris commented Nov 12, 2017

JackDevine commented Nov 12, 2017

andyferris commented Nov 12, 2017

JeffBezanson Nov 14, 2017

Choose a reason for hiding this comment

JeffBezanson Nov 14, 2017

Choose a reason for hiding this comment

JeffBezanson Nov 14, 2017

Choose a reason for hiding this comment

JeffBezanson Nov 14, 2017

Choose a reason for hiding this comment

JeffBezanson Nov 14, 2017

Choose a reason for hiding this comment

JackDevine commented Nov 14, 2017

JackDevine commented Jan 4, 2018

ararslan commented Jan 4, 2018

`in` uses `isequal` (#9381) #24563

`in` uses `isequal` (#9381) #24563

JackDevine commented Nov 10, 2017 •

edited by fredrikekre

Loading

StefanKarpinski commented Nov 10, 2017 •

edited

Loading