On Wed, Jul 17, 2019 at 03:43:11PM +0800, Peter Xu wrote: >On Wed, Jul 17, 2019 at 03:11:14PM +0800, Wei Yang wrote: >> Add a test for bitmap_set. There are three cases: >> >> * Both start and end is BITS_PER_LONG aligned >> * Only start is BITS_PER_LONG aligned >> * Only end is BITS_PER_LONG aligned >> >> Signed-off-by: Wei Yang <richardw.y...@linux.intel.com> > >Hi, Wei, > >Thanks for doing this. I've got a few comments below. > >> --- >> tests/test-bitmap.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/tests/test-bitmap.c b/tests/test-bitmap.c >> index cb7c5e462d..1f0123f604 100644 >> --- a/tests/test-bitmap.c >> +++ b/tests/test-bitmap.c >> @@ -59,12 +59,45 @@ static void check_bitmap_copy_with_offset(void) >> g_free(bmap3); >> } >> >> +static void check_bitmap_set(void) >> +{ >> + unsigned long *bmap; >> + >> + bmap = bitmap_new(BMAP_SIZE); > >Need to free this. >
oops, you are right. >> + >> + /* Both Aligned, set bits [BITS_PER_LONG, 2*BITS_PER_LONG] */ >> + bitmap_set(bmap, BITS_PER_LONG, BITS_PER_LONG); >> + g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG), ==, BITS_PER_LONG); > >Can check all 1's set correctly. > > g_assert_cmpuint(bmap[1], ==, -1ul); > >Can also make this at least across multiple long fields. > good suggestion >> + g_assert_cmpint(find_next_zero_bit(bmap, 2 * BITS_PER_LONG, >> BITS_PER_LONG), >> + ==, 2 * BITS_PER_LONG); >> + >> + bitmap_clear(bmap, 0, BMAP_SIZE); >> + /* End Aligned, set bits [BITS_PER_LONG - 5, 2*BITS_PER_LONG] */ >> + bitmap_set(bmap, BITS_PER_LONG - 5, BITS_PER_LONG + 5); > >Same here. > >> + g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG), >> + ==, BITS_PER_LONG - 5); >> + g_assert_cmpint(find_next_zero_bit(bmap, >> + 2 * BITS_PER_LONG, BITS_PER_LONG - >> 5), >> + ==, 2 * BITS_PER_LONG); >> + >> + bitmap_clear(bmap, 0, BMAP_SIZE); >> + /* Start Aligned, set bits [BITS_PER_LONG, 2*BITS_PER_LONG + 5] */ >> + bitmap_set(bmap, BITS_PER_LONG, BITS_PER_LONG + 5); > >And here. > >> + g_assert_cmpint(find_first_bit(bmap, BITS_PER_LONG), >> + ==, BITS_PER_LONG); >> + g_assert_cmpint(find_next_zero_bit(bmap, >> + 2 * BITS_PER_LONG + 5, >> BITS_PER_LONG), >> + ==, 2 * BITS_PER_LONG + 5); >> +} >> + >> int main(int argc, char **argv) >> { >> g_test_init(&argc, &argv, NULL); >> >> g_test_add_func("/bitmap/bitmap_copy_with_offset", >> check_bitmap_copy_with_offset); >> + g_test_add_func("/bitmap/bitmap_set", >> + check_bitmap_set); > >Can at least do the same test to bitmap_set_atomic too, simply by >allowing your helper test function to take a func pointer: > >void (*bmap_set_func)(unsigned long *map, long i, long len); > >Then call with both bitmap_set{_atomic}. > ok, let me take a look into this. >Thanks, > >-- >Peter Xu -- Wei Yang Help you, Help me