Hello Jon Olav!
My comments about new version of your patch are mostly cosmetic:
* Jon Olav Hauglid <jon.hauglid@stripped> [11/02/14 12:32]:
> #At file:///export/home/x/mysql-5.5-bug43152/ based on
> revid:georgi.kodinov@stripped
>
> 3324 Jon Olav Hauglid 2011-02-14
> Bug #11752069 (former bug 43152)
> Assertion `bitmap_is_set_all(&table->s->all_set)' failed in
> handler::ha_reset
...
> @ include/my_bit.h
> Removed now unused function.
Maybe it makes sense to mention in the file comment that you've also
introduced new function?
...
> === modified file 'mysys/my_bitmap.c'
> --- a/mysys/my_bitmap.c 2011-01-11 09:07:37 +0000
> +++ b/mysys/my_bitmap.c 2011-02-14 09:28:11 +0000
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 2000 MySQL AB, 2008-2009 Sun Microsystems, Inc
> +/* Copyright (c) 2000, 2011, Oracle and/or its affiliates. All rights reserved.
>
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
> @@ -92,6 +92,46 @@ static inline void bitmap_unlock(MY_BITM
> mysql_mutex_unlock(map->mutex);
> }
>
> +static inline uint get_first_set(uint32 value, uint word_pos)
> +{
AFAIR our coding style says that functions should be separated with
two empty lines.
> + uchar *byte_ptr;
> + uint byte_pos, bit_pos;
> + if (value)
> + {
Hmm.. Actually, I think, it is better to have this if-statement
outside this function, in the caller. See more below.
> + byte_ptr= (uchar*)&value;
> + for (byte_pos=0; byte_pos < 4; byte_pos++, byte_ptr++)
> + {
> + if (*byte_ptr)
> + {
> + for (bit_pos=0; bit_pos < 8; bit_pos++)
Note that "bit_pos < 8" condition is redundant here.
Please consider removing it.
> + if (*byte_ptr & (1 << bit_pos))
> + return (word_pos*32) + (byte_pos*8) + bit_pos;
> + }
> + }
> + }
> + return MY_BIT_NONE;
> +}
> +
> +static inline uint get_first(uint32 value, uint word_pos)
> +{
Similar comment about two empty lines here.
> + uchar *byte_ptr;
> + uint byte_pos, bit_pos;
> + if (value != 0xFFFFFFFF)
> + {
Similar comment about this if-statement as well.
> + byte_ptr= (uchar*)&value;
> + for (byte_pos=0; byte_pos < 4; byte_pos++, byte_ptr++)
> + {
> + if (*byte_ptr != 0xFF)
> + {
> + for (bit_pos=0; bit_pos < 8; bit_pos++)
And similar comment about "bit_pos < 8" condition here.
> + if (!(*byte_ptr & (1 << bit_pos)))
> + return (word_pos*32) + (byte_pos*8) + bit_pos;
> + }
> + }
> + }
> + return MY_BIT_NONE;
> +}
> +
>
> my_bool bitmap_init(MY_BITMAP *map, my_bitmap_map *buf, uint n_bits,
> my_bool thread_safe __attribute__((unused)))
> @@ -259,28 +299,38 @@ void bitmap_set_prefix(MY_BITMAP *map, u
>
> my_bool bitmap_is_prefix(const MY_BITMAP *map, uint prefix_size)
> {
> - uint prefix_bits= prefix_size & 0x7, res;
> - uchar *m= (uchar*)map->bitmap;
> - uchar *end_prefix= m+prefix_size/8;
> - uchar *end;
> - DBUG_ASSERT(m && prefix_size <= map->n_bits);
> - end= m+no_bytes_in_map(map);
> -
> - while (m < end_prefix)
> - if (*m++ != 0xff)
> - return 0;
> -
> - *map->last_word_ptr&= ~map->last_word_mask; /*Clear bits*/
> - res= 0;
> - if (prefix_bits && *m++ != (1 << prefix_bits)-1)
> - goto ret;
> -
> - while (m < end)
> - if (*m++ != 0)
> - goto ret;
> - res= 1;
> -ret:
> - return res;
> + uint prefix_bits= prefix_size % 32;
> + my_bitmap_map *word_ptr= map->bitmap, last_word;
> + my_bitmap_map *end_prefix= word_ptr + prefix_size / 32;
> + DBUG_ASSERT(word_ptr && prefix_size <= map->n_bits);
> +
> + /* 1: Words that should be filled with 1 */
> + while (word_ptr < end_prefix)
> + if (*word_ptr++ != 0xFFFFFFFF)
> + return FALSE;
> +
> + last_word= *map->last_word_ptr & ~map->last_word_mask;
> +
> + /* 2: Word which contains the end of the prefix (if any) */
> + if (prefix_bits)
> + {
> + if (word_ptr == map->last_word_ptr)
> + return uint4korr((uchar*)&last_word) == (uint32)((1 << prefix_bits)
> - 1);
> + else if (uint4korr((uchar*)word_ptr) != (uint32)((1 << prefix_bits) - 1))
> + return FALSE;
> + word_ptr++;
> + }
> +
> + /* 3: Words that should be filled with 0 */
> + while (word_ptr < map->last_word_ptr)
> + if (*word_ptr++ != 0)
> + return FALSE;
> +
> + /*
> + word_ptr will be larger than map->last_word_ptr if
> + prefix_size equals map->n_bits and the bitmap is filled with 1.
> + */
> + return word_ptr > map->last_word_ptr || last_word == 0;
> }
Hmm... OK. But I think it is also worth to mention in the above comment
that word_ptr > map->last_word_ptr is possible only when prefix % 32 == 0
(after you have added a shortcut for case when word_ptr == last_word_ptr).
>
>
...
> @@ -478,65 +538,39 @@ void bitmap_copy(MY_BITMAP *map, const M
>
> uint bitmap_get_first_set(const MY_BITMAP *map)
> {
> - uchar *byte_ptr;
> - uint i,j,k;
> + uint word_pos, bit_pos;
> my_bitmap_map *data_ptr, *end= map->last_word_ptr;
>
> DBUG_ASSERT(map->bitmap);
> data_ptr= map->bitmap;
> - *map->last_word_ptr &= ~map->last_word_mask;
>
> - for (i=0; data_ptr <= end; data_ptr++, i++)
> + for (word_pos=0; data_ptr < end; data_ptr++, word_pos++)
> {
> - if (*data_ptr)
> - {
> - byte_ptr= (uchar*)data_ptr;
> - for (j=0; ; j++, byte_ptr++)
> - {
> - if (*byte_ptr)
> - {
> - for (k=0; ; k++)
> - {
> - if (*byte_ptr & (1 << k))
> - return (i*32) + (j*8) + k;
> - }
> - }
> - }
> - }
> + bit_pos= get_first_set(*data_ptr, word_pos);
> + if (bit_pos != MY_BIT_NONE)
> + return bit_pos;
As I have said above, IMO, it makes sense to move "if (*data_ptr)"
out of get_first_set() call. This will make the above "if (bit_pos)"
unnecessary.
> }
> - return MY_BIT_NONE;
> +
> + return get_first_set(*map->last_word_ptr & ~map->last_word_mask,
> word_pos);
> }
>
>
> uint bitmap_get_first(const MY_BITMAP *map)
> {
> - uchar *byte_ptr;
> - uint i,j,k;
> + uint word_pos, bit_pos;
> my_bitmap_map *data_ptr, *end= map->last_word_ptr;
>
> DBUG_ASSERT(map->bitmap);
> data_ptr= map->bitmap;
> - *map->last_word_ptr|= map->last_word_mask;
>
> - for (i=0; data_ptr <= end; data_ptr++, i++)
> + for (word_pos=0; data_ptr < end; data_ptr++, word_pos++)
> {
> - if (*data_ptr != 0xFFFFFFFF)
> - {
> - byte_ptr= (uchar*)data_ptr;
> - for (j=0; ; j++, byte_ptr++)
> - {
> - if (*byte_ptr != 0xFF)
> - {
> - for (k=0; ; k++)
> - {
> - if (!(*byte_ptr & (1 << k)))
> - return (i*32) + (j*8) + k;
> - }
> - }
> - }
> - }
> + bit_pos= get_first(*data_ptr, word_pos);
> + if (bit_pos != MY_BIT_NONE)
> + return bit_pos;
Same comment here.
> }
> - return MY_BIT_NONE;
> +
> + return get_first(*map->last_word_ptr | map->last_word_mask, word_pos);
> }
I think it is OK to push this patch after considering the above comments
and getting approval from the second reviewer.
--
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
