List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:November 5 2010 3:54pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (guilhem:3330) Bug#57928
View as plain text  
Thanks Guilhem, patches look good, only some nitpicks.

-- didrik


-static int sort_keyuse(KEYUSE *a,KEYUSE *b);
-static void set_position(JOIN *join,uint index,JOIN_TAB *table,KEYUSE *key);
-static bool create_ref_for_key(JOIN *join, JOIN_TAB *j, KEYUSE *org_keyuse,
+static int sort_keyuse(Key_use *a, Key_use *b);
+static void set_position(JOIN *join, uint index, JOIN_TAB *table,
+                         Key_use *key);

No need to split this in two lines.

-  bool          null_rejecting;
-  bool          *cond_guard; /* See KEYUSE::cond_guard */
-  uint          sj_pred_no; /* See KEYUSE::sj_pred_no */
+  bool          null_rejecting;
+  bool          *cond_guard; ///< @sa Key_use::cond_guard
+  uint          sj_pred_no;  ///< @sa Key_use::sj_pred_no

(setq comment-column 48)

+  TABLE *table;  ///< table owning the index
+  Item	*val;    ///< other side of the equality, or value if no field
+  table_map used_tables; ///< tables used on other side of equality
+  uint key;      ///< number of index
+  uint keypart;  ///< used part of the index
+  uint optimize; ///< 0, or KEY_OPTIMIZE_*
+  key_part_map keypart_map; ///< like keypart, but as a bitmap
+  ha_rows      ref_table_rows; ///< Estimate of how many rows for a key value


Thanks for commenting, could you try to align (most of) them?


On Thu, Nov 4, 2010 at 6:11 PM, Guilhem Bichot <guilhem.bichot@stripped>wrote:

> 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