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.

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

Jump to bottom

Tests for IntSet. Includes two bug fixes #12247

Merged
merged 2 commits into from
Jul 22, 2015
Merged

Tests for IntSet. Includes two bug fixes #12247

merged 2 commits into from
Jul 22, 2015

Conversation

dpsanders
Copy link
Contributor

No description provided.

@dpsanders dpsanders mentioned this pull request Jul 21, 2015
Closed
2 tasks
@dpsanders
Copy link
Contributor Author

Ping @mbauman @kshyatt

mbauman
mbauman reviewed Jul 21, 2015
View reviewed changes
base/intset.jl
@@ -274,7 +274,7 @@ function ==(s1::IntSet, s2::IntSet)
return false
end
end
filln = s1.fill1s ? UInt32(-1) : UInt32(0)
filln = s1.fill1s ? reinterpret(UInt32, Int32(-1)) : UInt32(0)
Copy link
Member

Choose a reason for hiding this comment

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

Hah, even stronger evidence that nobody is using complements (at least not on master).

I would spell this 0xffffffff or -1 % UInt32, but it's not a big deal since Julia/LLVM can figure all three out and inlines them all down to the constant.

@mbauman
Copy link
Member

mbauman commented Jul 21, 2015

Nice work. 👍

@kshyatt kshyatt added the test This change adds or pertains to unit tests label Jul 21, 2015
@kshyatt
Copy link
Contributor

kshyatt commented Jul 21, 2015

This looks good. Would it be possible to rebase a bit so all the new tests show up in one commit? E.g. so that "First additional", "Finish", and "Refine" are all in one commit? @mbauman does that sound reasonable to you?

@dpsanders
Copy link
Contributor Author

Sure, I'll rebase when I get a moment.

@dpsanders
Copy link
Contributor Author

By the way, I was thinking of adding a constructor to be able to do IntSet(1, 2, 3) instead of IntSet([1, 2, 3]). This was deprecated in 0.3 apparently -- is this a bad idea @mbauman?

@mbauman
Copy link
Member

mbauman commented Jul 21, 2015

Two commits sounds logical (move tests + your additions).

The deprecated constructor is so all set-like constructors take just one iterable. If we have both, there's an ambiguity with one argument: does Set(x) iterate over x? Or does it construct a one-element set with just x itself? For IntSet there is no ambiguity, but it's nice to be consistent with Set.

@mbauman
Copy link
Member

mbauman commented Jul 21, 2015

Hm, actually, it doesn't look like it was deprecated in 0.3. It was just straight-up removed?

@dpsanders
Copy link
Contributor Author

It's in deprecated.jl but was not actually deprecated (still works on 0.3.11pre).

I see the point about constructors. I'll squash down to just 2 commits.

@dpsanders
Fix 2 bugs in base/intset.jl
4af13cf 
Correct untested bug in symdiff! in intset.jl

Fix a bug in intset
@dpsanders
Move IntSet tests from sets.jl to intset.jl and increase coverage
562eac0 
Moved existing IntSet tests from sets.jl to intset.jl

First additional tests for intset

Finish intset tests

Refine intset tests
@dpsanders
Copy link
Contributor Author

This is ready once Travis turns green.

@mbauman For the constructor, how about the following?

Set(a, b...) = Set((a, b...))
IntSet(a, b...) = IntSet((a, b...))

This allows e.g.

julia> Set([1,2], [3,4])
Set([[1,2],[3,4]])

@mbauman
Copy link
Member

mbauman commented Jul 22, 2015

Then what does Set([1,2]) mean? Why would that be a set of two integers if Set([1,2],[3,4]) is a set of two arrays? See #5897 for more discussion.

@dpsanders
Copy link
Contributor Author

True, good point. Sorry for the noise.

@mbauman
Copy link
Member

mbauman commented Jul 22, 2015

No worries. This looks great. Thanks!

@dpsanders
Copy link
Contributor Author

Glad you like it, thanks!

mbauman added a commit that referenced this pull request Jul 22, 2015
@mbauman
Merge pull request #12247 from dpsanders/dps/test_intset
43b7a49 
Tests for IntSet. Includes two bug fixes
@mbauman mbauman merged commit 43b7a49 into JuliaLang:master Jul 22, 2015
@dpsanders dpsanders deleted the dps/test_intset branch July 22, 2015 15:50
@catawbasam
Copy link
Contributor

Looks like Coveralls isn't updating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dpsanders @mbauman @kshyatt @catawbasam