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

Sets coverage. #11795

Merged
merged 1 commit into from
Jun 26, 2015
Merged

Sets coverage. #11795

merged 1 commit into from
Jun 26, 2015

Conversation

Ismael-VC
Copy link
Contributor

Add test for Base.rehash!(s::Set{T}).

Add test for `rehash!(s::Set{T})`.
@Ismael-VC
Copy link
Contributor Author

Ref: julia-dev

@JeffBezanson
Copy link
Member

Thanks for helping with test coverage!

Honestly I'm not sure why there is a rehash! method for Set. Maybe we should just remove it?

@Ismael-VC
Copy link
Contributor Author

You are welcome!

I've been reading about hashing and it's use in associative arrays, etc. But honestly I don't know if
Base.rehash!(s::Set{T}) is really useful or not for sets.

I'm just trying to change those red lines to green at coveralls. 😄

@Ismael-VC
Copy link
Contributor Author

@JeffBezanson what I understood was that rehash! is issued every time a Set grows or shrinks, but since a Dict is used internally, it seems to me that that would call rehash!(d::Dict{K, V}, newsz).

The cpp reference seems to also have something like this defined for sets. Perhaps there is indeed a use case? But does it apply to Julia also?

@Ismael-VC
Copy link
Contributor Author

Honestly I'm not sure why there is a rehash! method for Set. Maybe we should just remove it?

@stevengj could you tell us why do we need Base.rehash!(s::Set{T})?

@StefanKarpinski
Copy link
Member

No idea. Did I introduce it?

@StefanKarpinski
Copy link
Member

Oh, wrong ping.

@yuyichao
Copy link
Contributor

@StefanKarpinski To be fair, it was @ing you in the original version.

@Ismael-VC
Copy link
Contributor Author

It was my mistake, I looked at git blame, and misread by one line the name of the author:

Which is why I changed it back. So it seems Steven never got the ping.

@stevengj could you please give us some insight on this method when yo have the chance?

He only changed the name from rehash to rehash! though, I'll try to track down who introduced it. But if no one knows why it's there, then I second Jeff about removing it.

I'm sorry for the noise.

cheers

@yuyichao
Copy link
Contributor

@Ismael-VC That is actually still not the right commit this e8d47e4 is the commit it was introduced and it was by @JeffBezanson .

@yuyichao
Copy link
Contributor

Sorry I was also wrong. It was e7ce4cb by @timholy

@Ismael-VC
Copy link
Contributor Author

Add convenience calls for rehashing

This is relevant particularly to precompilation, which requires
that any module global dicts get rehashed by the __init__
function.

I think this still stands then.

@stevengj
Copy link
Member

Yes, it was for precompilation.

@Ismael-VC
Copy link
Contributor Author

Ok! then, is the test good as it is?

@stevengj
Copy link
Member

I suppose so; I guess there's no way to test rehash! without digging into the Set internals.

@StefanKarpinski
Copy link
Member

Bump. Should we go ahead with this?

stevengj added a commit that referenced this pull request Jun 26, 2015
@stevengj stevengj merged commit 56dfd01 into JuliaLang:master Jun 26, 2015
@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

Looks like this introduced a non-deterministic failure, see #11890. Happens sometimes, so not a big deal, we'll work out what's best to do here while hopefully keeping this method covered for correctness if we can.

@yuyichao yuyichao mentioned this pull request Jun 27, 2015
@tkelman
Copy link
Contributor

tkelman commented Jun 27, 2015

should be fixed by #11892

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.

6 participants