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

Count() can be wrong when used on a copied bitset. #112

Closed
steampoweredtaco opened this issue Oct 27, 2022 · 1 comment · Fixed by #113
Closed

Count() can be wrong when used on a copied bitset. #112

steampoweredtaco opened this issue Oct 27, 2022 · 1 comment · Fixed by #113

Comments

@steampoweredtaco
Copy link
Contributor

version: v1.3.3
Issue
I've found an issue where Count() may return the wrong count if the BitSet had been copied from another BitSet. It appears that there are some edge cases that had set bits that overflow the length of the copied to BitSet in the last uint64 those will get counted.

I think a solution would be to have the Copy method set the bits that overflow from the copied to count to 0.

This test case makes this obvious, Count() > Len() in the copy and notice how the test that takes a copy of 64 bits from a 72 bitset passes. That is because the copies target count is aligned to a 64 bit boundary so there is no room for the extra set bits from the source copy to pollute the result.

Test:

package main

import (
	"fmt"
	"github.com/bits-and-blooms/bitset"
	"testing"
)

func Test_bitsetCount(t *testing.T) {
	type args struct {
		size uint
	}
	type want struct {
		size uint
	}
	type testRow struct {
		name string
		args args
		want want
	}
	tests := make([]testRow, 15)
	for i := range tests {
		tests[i].name = fmt.Sprintf("size_%d_copied_to_%d", 8*(i+1), 8*(i+1)-8)
		tests[i].args.size = uint(i+1) * 8
		tests[i].want.size = uint(i+1) * 8
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			res := bitset.New(tt.args.size)
			if res.Len() != tt.want.size {
				t.Errorf("bitset size: %d, want %+v", res.Len(), tt.want.size)
			}
			res.FlipRange(0, res.Len())
			if res.Count() != tt.want.size || res.Count() != res.Len() {
				t.Errorf("bitsize count: %d, want %d", res.Count(), tt.want.size)
			}
			cpy := bitset.New(res.Len() - 8)
			res.Copy(cpy)
			if cpy.Count() != tt.want.size-8 || cpy.Count() != cpy.Len() {
				t.Errorf("bitsize count: %d of %d, want %d", cpy.Count(), cpy.Len(), tt.want.size-8)
			}
		})
	}
}

Results:

=== RUN   Test_bitsetCount
=== RUN   Test_bitsetCount/size_8_copied_to_0
=== RUN   Test_bitsetCount/size_16_copied_to_8
    qr_test.go:76: bitsize count: 16 of 8, want 8
=== RUN   Test_bitsetCount/size_24_copied_to_16
    qr_test.go:76: bitsize count: 24 of 16, want 16
=== RUN   Test_bitsetCount/size_32_copied_to_24
    qr_test.go:76: bitsize count: 32 of 24, want 24
=== RUN   Test_bitsetCount/size_40_copied_to_32
    qr_test.go:76: bitsize count: 40 of 32, want 32
=== RUN   Test_bitsetCount/size_48_copied_to_40
    qr_test.go:76: bitsize count: 48 of 40, want 40
=== RUN   Test_bitsetCount/size_56_copied_to_48
    qr_test.go:76: bitsize count: 56 of 48, want 48
=== RUN   Test_bitsetCount/size_64_copied_to_56
    qr_test.go:76: bitsize count: 64 of 56, want 56
=== RUN   Test_bitsetCount/size_72_copied_to_64
=== RUN   Test_bitsetCount/size_80_copied_to_72
    qr_test.go:76: bitsize count: 80 of 72, want 72
=== RUN   Test_bitsetCount/size_88_copied_to_80
    qr_test.go:76: bitsize count: 88 of 80, want 80
=== RUN   Test_bitsetCount/size_96_copied_to_88
    qr_test.go:76: bitsize count: 96 of 88, want 88
=== RUN   Test_bitsetCount/size_104_copied_to_96
    qr_test.go:76: bitsize count: 104 of 96, want 96
=== RUN   Test_bitsetCount/size_112_copied_to_104
    qr_test.go:76: bitsize count: 112 of 104, want 104
=== RUN   Test_bitsetCount/size_120_copied_to_112
    qr_test.go:76: bitsize count: 120 of 112, want 112
--- FAIL: Test_bitsetCount (0.00s)
    --- PASS: Test_bitsetCount/size_8_copied_to_0 (0.00s)
    --- FAIL: Test_bitsetCount/size_16_copied_to_8 (0.00s)

    --- FAIL: Test_bitsetCount/size_24_copied_to_16 (0.00s)

    --- FAIL: Test_bitsetCount/size_32_copied_to_24 (0.00s)

    --- FAIL: Test_bitsetCount/size_40_copied_to_32 (0.00s)

    --- FAIL: Test_bitsetCount/size_48_copied_to_40 (0.00s)

    --- FAIL: Test_bitsetCount/size_56_copied_to_48 (0.00s)

    --- FAIL: Test_bitsetCount/size_64_copied_to_56 (0.00s)

    --- PASS: Test_bitsetCount/size_72_copied_to_64 (0.00s)
    --- FAIL: Test_bitsetCount/size_80_copied_to_72 (0.00s)

    --- FAIL: Test_bitsetCount/size_88_copied_to_80 (0.00s)

    --- FAIL: Test_bitsetCount/size_96_copied_to_88 (0.00s)

    --- FAIL: Test_bitsetCount/size_104_copied_to_96 (0.00s)

    --- FAIL: Test_bitsetCount/size_112_copied_to_104 (0.00s)

    --- FAIL: Test_bitsetCount/size_120_copied_to_112 (0.00s)


FAIL


Process finished with the exit code 1


@steampoweredtaco
Copy link
Contributor Author

Looks like a simple fix using an existing method at the end of copy. I added cleanLastWord and a non-exhaustive unit test that should be good enough to prove the fix and fails without it.

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 a pull request may close this issue.

1 participant