MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:June 17 2008 9:36am
Subject:Re: bk commit into 6.0 tree (aelkin:1.2634) BUG#25998
View as plain text  
Hi Andrei!

Patch approved, but see my comments below.

Just my few cents,
Mats Kindahl

Andrei Elkin wrote:
[snip]

>  
> +/**
> +   A comparison function to be supplied as argument to @c sort_dynamic()
> +   and @c bsearch()
> +
> +   @return -1 if first argument is less, 0 if it equal to, 1 if it is greater
> +   than the second
> +*/
> +int change_master_server_id_cmp(ulong *id1, ulong *id2)
> +{
> +  return *id1 < *id2? -1 : (*id1 > *id2? 1 : 0);
> +}

I usually prefer to put the casts inside the comparison function and not
use a cast when passing the function. However, you cannot do that since
the two comparison functions have different signatures.

> +
> +
> +/**
> +   Reports if the s_id server has been configured to ignore events 
> +   it generates with
> +
> +      CHANGE MASTER IGNORE_SERVER_IDS= ( list of server ids )
> +
> +   Method is called from the io thread event receiver filtering.
> +
> +   @param      s_id    the master server identifier
> +
> +   @retval   TRUE    if s_id is in the list of ignored master  servers,
> +   @retval   FALSE   otherwise.
> + */
> +bool Master_info::shall_ignore_server_id(ulong s_id)
> +{
> +  if (likely(ignore_server_ids.elements == 1))
> +    return (* (ulong*) dynamic_array_ptr(&ignore_server_ids, 0)) == s_id;
> +  else      
> +    return bsearch((const ulong *) &s_id,

You don't need this cast, casts to (void*) can be done implicitly. Casts
are evil: they hide errors are are almost never needed, assuming
surrounding code is written correctly.

> +                   ignore_server_ids.buffer,
> +                   ignore_server_ids.elements, sizeof(ulong),
> +                   (int (*) (const void*, const void*))
> change_master_server_id_cmp)

Here, I would prefer to not have an explicit cast, but that cannot be
easily accommodated since the comparison functions for DYNAMIC_ARRAY and
bsearch() have different signatures.


[snip]

> +      cur_len +=my_sprintf(ignore_server_ids_buf + cur_len,

space is on wrong side of +=

[snip]

> +      if (bsearch((const ulong *) &s_id,
> +                  mi->ignore_server_ids.buffer,
> +                  mi->ignore_server_ids.elements, sizeof(ulong),
> +                  (int (*) (const void*, const void*))
> +                  change_master_server_id_cmp) == NULL)
> +        insert_dynamic(&mi->ignore_server_ids, (uchar*) &s_id);
> +    }
> +  }
> +  sort_dynamic(&mi->ignore_server_ids, (qsort_cmp)
> change_master_server_id_cmp);

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com

Thread
bk commit into 6.0 tree (aelkin:1.2634) BUG#25998Andrei Elkin4 Jun
  • Re: bk commit into 6.0 tree (aelkin:1.2634) BUG#25998Mats Kindahl17 Jun
  • Re: bk commit into 6.0 tree (aelkin:1.2634) BUG#25998He Zhenxing2 Jul