List:Commits« Previous MessageNext Message »
From:Alexander Nozdrin Date:April 5 2011 11:06am
Subject:Re: bzr commit into mysql-trunk branch (Dmitry.Lenev:3536) Bug#11746602
View as plain text  
Hi Dmitry,

thank you for working on this.

I have a few minor comments, otherwise the patch is Ok to push.

On 04/04/11 21:38, Dmitry Lenev wrote:
>       @ sql/sql_lex.h
>          Since HANDLER READ command is now respresented by
                                              ^^^^^^ typo
>          class inhereted from Sql_cmd there is no need in
                  ^^^^^^^ typo

> === modified file 'sql/sql_handler.cc'
> --- a/sql/sql_handler.cc	2011-03-26 10:56:27 +0000
> +++ b/sql/sql_handler.cc	2011-04-04 17:38:43 +0000
> @@ -60,12 +60,15 @@
>   #include "sql_base.h"                           // insert_fields
>   #include "sql_select.h"
>   #include "transaction.h"
> +#include "sql_parse.h"                          // check_table_access
>
>   #define HANDLER_TABLES_HASH_SIZE 120
>
>   static enum enum_ha_read_modes rkey_to_rnext[]=
>   { RNEXT_SAME, RNEXT, RPREV, RNEXT, RPREV, RNEXT, RPREV, RPREV };
>
> +static bool mysql_ha_open_table(THD *thd, TABLE_LIST *table);
> +

May be put it right before Sql_cmd_handler_open implementation?

> === modified file 'sql/sql_handler.h'
> --- a/sql/sql_handler.h	2010-11-18 16:34:56 +0000
> +++ b/sql/sql_handler.h	2011-04-04 17:38:43 +0000
> @@ -23,10 +23,100 @@
>   class THD;
>   struct TABLE_LIST;
>
> -bool mysql_ha_open(THD *thd, TABLE_LIST *tables, bool reopen);
> -bool mysql_ha_close(THD *thd, TABLE_LIST *tables);
> -bool mysql_ha_read(THD *, TABLE_LIST *,enum enum_ha_read_modes,char *,
> -                   List<Item>  *,enum ha_rkey_function,Item
> *,ha_rows,ha_rows);
> +/**
> +  Sql_cmd_handler_open represents HANDLER OPEN statement.
> +
> +  @note Some information about this statement, for example, table to be
> +        opened is still kept in LEX class.
> +*/
> +
> +class Sql_cmd_handler_open : public Sql_cmd
> +{
> +public:
> +  Sql_cmd_handler_open()
> +  {}
> +
> +  virtual ~Sql_cmd_handler_open()
> +  {}
> +
> +  virtual enum_sql_command sql_command_code() const
> +  {
> +    return SQLCOM_HA_OPEN;
> +  }
> +
> +  bool execute(THD *thd);


Could you please add 'virtual' modifier to execute() here
and everywhere? It's not needed for the compiler,
but it improves understanding when reading the code.
It's also useful for understanding 'grep execute' output.

> +private:
> +  /** Read mode for HANDLER READ: FIRST, NEXT, LAST, ... */
> +  enum enum_ha_read_modes m_read_mode;
> +  /**
> +    Name of key to be used for reading,
> +    NULL in cases when natural row-order is to be used.
> +  */
> +  const char *m_key_name;
> +  /** Key values to be satisfied. */
> +  List<Item>  *m_key_expr;
> +  /** Type of condition for key values to be satisfied. */
> +  enum ha_rkey_function m_rkey_mode;
> +};

Please consider separating declarations by an empty line
to improve readability.

I.e.:
 > +  /** Read mode for HANDLER READ: FIRST, NEXT, LAST, ... */
 > +  enum enum_ha_read_modes m_read_mode;

 > +  /**
 > +    Name of key to be used for reading,
 > +    NULL in cases when natural row-order is to be used.
 > +  */
 > +  const char *m_key_name;

 > +  /** Key values to be satisfied. */
 > +  List<Item>  *m_key_expr;

 > +  /** Type of condition for key values to be satisfied. */
 > +  enum ha_rkey_function m_rkey_mode;

>   handler_rkey_function:
> -          FIRST_SYM { Lex->ha_read_mode = RFIRST; }
> -        | NEXT_SYM  { Lex->ha_read_mode = RNEXT;  }
> -        | PREV_SYM  { Lex->ha_read_mode = RPREV;  }
> -        | LAST_SYM  { Lex->ha_read_mode = RLAST;  }
> +          FIRST_SYM { $$= RFIRST; }
> +        | NEXT_SYM  { $$= RNEXT;  }
> +        | PREV_SYM  { $$= RPREV;  }
> +        | LAST_SYM  { $$= RLAST;  }
>           | handler_rkey_mode
>             {
> -            LEX *lex=Lex;
> -            lex->ha_read_mode = RKEY;
> -            lex->ha_rkey_mode=$1;
> -            if (!(lex->insert_list = new List_item))
> +            YYTHD->m_parser_state->m_yacc.m_ha_rkey_mode= $1;
> +            if (!(Lex->insert_list = new List_item))

Could you please separate that into assignment and check?
Like that:

Lex->insert_list= new List_item();
if (!Lex->insert_list)
   MYSQL_YYABORT;
Thread
bzr commit into mysql-trunk branch (Dmitry.Lenev:3536) Bug#11746602Dmitry Lenev4 Apr
  • Re: bzr commit into mysql-trunk branch (Dmitry.Lenev:3536) Bug#11746602Alexander Nozdrin5 Apr