List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 4 2010 5:11pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)
Bug#57928
View as plain text  
Hello Jorgen and Tor,

Tor Didriksen a écrit, Le 04.11.2010 13:27:
> 
> 
> On Wed, Nov 3, 2010 at 2:54 PM, Guilhem Bichot <guilhem@stripped 
> <mailto:guilhem@stripped>> wrote:
> 
>     #At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-bugfixing2/
>     based on revid:guilhem@stripped
> 
>      3330 Guilhem Bichot    2010-11-03
>          Fix for BUG#57928 "More Valgrind warnings in print_keyuse()
>     with --debug":
>          for a fulltext index, the KEYUSE object had some uninitialized
>     members.
>     === modified file 'sql/sql_select.cc'
>     --- a/sql/sql_select.cc 2010-10-21 13:30:19 +0000
>     +++ b/sql/sql_select.cc 2010-11-03 13:54:05 +0000
>     @@ -5861,20 +5861,15 @@ add_key_part(DYNAMIC_ARRAY *keyuse_array
>           {
>            if (field->eq(form->key_info[key].key_part[part].field))
>            {
>     -          KEYUSE keyuse;
>     -          keyuse.table=          field->table;
>     -          keyuse.val=            key_field->val;
>     -          keyuse.used_tables=    key_field->val->used_tables();
>     -          keyuse.key=            key;
>     -          keyuse.keypart=        part;
>     -          keyuse.optimize=       key_field->optimize &
>     KEY_OPTIMIZE_REF_OR_NULL;
>     -          keyuse.keypart_map=    (key_part_map) 1 << part;
>     -          /* This will be set accordingly in optimize_keyuse */
>     -          keyuse.ref_table_rows= ~(ha_rows) 0;
>     -          keyuse.null_rejecting= key_field->null_rejecting;
>     -          keyuse.cond_guard=     key_field->cond_guard;
>     -          keyuse.sj_pred_no=     key_field->sj_pred_no;
>     -          if (insert_dynamic(keyuse_array, (uchar*) &keyuse))
>     +          const KEYUSE keyuse(field->table, key_field->val,
>     +                              key_field->val->used_tables(), key, part,
>     +                              key_field->optimize &
>     KEY_OPTIMIZE_REF_OR_NULL,
>     +                              (key_part_map) 1 << part,
>     +                              ~(ha_rows) 0, // will be set in
>     optimize_keyuse
>     +                              key_field->null_rejecting,
>     +                              key_field->cond_guard,
>     +                              key_field->sj_pred_no);
>     +          if (insert_dynamic(keyuse_array, &keyuse))
>                 return TRUE;
>            }
>           }
> 
> 
> I prefer
> 
>           const KEYUSE keyuse(field->table,
>                               key_field->val,
>                               key_field->val->used_tables(),
>                               key,
>                               part,
>                               key_field->optimize & 
> KEY_OPTIMIZE_REF_OR_NULL,
>                               (key_part_map) 1 << part,
>                               ~(ha_rows) 0, // will be set in 
> optimize_keyuse
>                               key_field->null_rejecting,
>                               key_field->cond_guard,
>                               key_field->sj_pred_no);

When I review, I rather annoy the developer asking him to squeeze on one 
line :-) Ok, the next patch will have one parameter per line.

> 
>  
> 
>     @@ -5938,16 +5933,11 @@ add_ft_keys(DYNAMIC_ARRAY *keyuse_array,
>           !(usable_tables & cond_func->table->map))
>         return FALSE;
> 
>     -  KEYUSE keyuse;
>     -  keyuse.table= cond_func->table;
>     -  keyuse.val =  cond_func;
>     -  keyuse.key =  cond_func->key;
>     -  keyuse.keypart= FT_KEYPART;
>     -  keyuse.used_tables=cond_func->key_item()->used_tables();
>     -  keyuse.optimize= 0;
>     -  keyuse.keypart_map= 0;
>     -  keyuse.sj_pred_no= UINT_MAX;
>     -  return insert_dynamic(keyuse_array,(uchar*) &keyuse);
>     +  const KEYUSE keyuse(cond_func->table, cond_func,
>     +                      cond_func->key_item()->used_tables(),
>     cond_func->key,
>     +                      FT_KEYPART, 0, 0, ~(ha_rows) 0, false, NULL,
>     +                      UINT_MAX);
>     +  return insert_dynamic(keyuse_array, &keyuse);
>      }
> 
> 
> I would *strongly* prefer
> 
>   const KEYUSE keyuse(cond_func->table,
>                       cond_func,
>                       cond_func->key_item()->used_tables(),
>                       cond_func->key,
>                       FT_KEYPART,
>                       0,                        // optimize
>                       0,                        // keypart_map
>                       ~(ha_rows) 0,             // ref_table_rows
>                       false,                    // null_rejecting
>                       NULL,                     // cond_guard
>                       UINT_MAX);                // sj_pred_no

done

>     === modified file 'sql/sql_select.h'
>     --- a/sql/sql_select.h  2010-09-23 12:16:36 +0000
>     +++ b/sql/sql_select.h  2010-11-03 13:54:05 +0000
>     @@ -39,17 +39,34 @@
>      #define KEY_OPTIMIZE_EXISTS            1
>      #define KEY_OPTIMIZE_REF_OR_NULL       2
> 
>     -typedef struct keyuse_t {
>     +/**
>     +  @note such objects are stored in DYNAMIC_ARRAY which uses
>     sizeof(), so keep
>     +  this class as POD as possible.
>     +*/
>     +class KEYUSE {
>     +public:
>     +  KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
>     +         uint key_arg, uint keypart_arg, uint optimize_arg,
>     +         key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
>     +         bool null_rejecting_arg, bool *cond_guard_arg,
>     +         uint sj_pred_no_arg) :
>     +  table(table_arg), val(val_arg), used_tables(used_tables_arg),
>     key(key_arg),
>     +  keypart(keypart_arg), optimize(optimize_arg),
>     keypart_map(keypart_map_arg),
>     +  ref_table_rows(ref_table_rows_arg),
>     null_rejecting(null_rejecting_arg),
>     +  cond_guard(cond_guard_arg), sj_pred_no(sj_pred_no_arg) 
> 
>     {}
> 
> 
> Please put the empty body on a separate line.

done

> And if you want to squish as many character as possible on each line,

oh yes!

> could you keep the argument list, and the initializers aligned?
> something like:
> 
>   KEYUSE(TABLE *table_arg, Item *val_arg, table_map used_tables_arg,
>          uint key_arg, uint keypart_arg, uint optimize_arg,
>          key_part_map keypart_map_arg, ha_rows ref_table_rows_arg,
>          bool null_rejecting_arg, bool *cond_guard_arg,
>          uint sj_pred_no_arg) :
>   table(table_arg), val(val_arg), used_tables(used_tables_arg),
>   key(key_arg), keypart(keypart_arg), optimize(optimize_arg),
>   keypart_map(keypart_map_arg), ref_table_rows(ref_table_rows_arg),
>   null_rejecting(null_rejecting_arg), cond_guard(cond_guard_arg),
>   sj_pred_no(sj_pred_no_arg)
>   {}

done

>     +  /** Estimate of how many rows for a key value */
> 
> 
> The style guide was recently extended (at your request :-) to allow
> // Estimate of how many rows for a key value.

Changed to /// (doxygen-compliant).

> Now we have a mix of five different comment styles, just for the member 
> fields of this class.

I will unify a bit the comment styles for those members in the next patch.

I'm quite happy to be allowed to write
// comment on a line even with no code on the left
now. Thanks a lot!

Also renamed KEYUSE to Key_use. That led to changes in many lines; I 
then used that as pretext to make those lines compliant with our coding 
style, deleting white space, adding space after equal, and other little 
things; so the new commit will be bigger than the first one.
The new commit is in two patches:
http://lists.mysql.com/commits/122859
changes insert_dynamic/set_dynamic;
then
http://lists.mysql.com/commits/122862
is incremental on it and fixes the bug.

Thread
bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Guilhem Bichot3 Nov
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot4 Nov
    • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928Tor Didriksen5 Nov
      • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot5 Nov
Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Jorgen Loland4 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330)Bug#57928Guilhem Bichot4 Nov