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.
