Re: bzr commit into MySQL Storage Engine API team tree:mysql-6.0-sea branch (v.narayanan:2643) WL#4448

[Narayanan]

Thanx for the detailed reviews/comments. I do not disagree with any of 
your comments.

mysql_priv.h.pp would need code refactoring. If you are OK with it, this 
can be handled as
part of 4394.

I shall watch the changeset comments in the future, thanx for the tip.

Narayanan

Sergei Golubchik wrote:
> Hi!
>
> Basically ok - the only problem - you forgot to remove
> fill_files_table() from the handlerton. Feel free to push after
> correcting this.
>
> And pay more attention in the future to changeset/file comments please -
> see my remarks below.
>
> On Jun 27, Narayanan V wrote:
>   
>> #At bzr+ssh://bk-internal.mysql.com/bzrroot/mysql-6.0-sea
>>
>>  2643 Narayanan V	2008-06-27
>>       WL#4448
>>       
>>       The attached patch adds a method handlerton::fill_is_table that can be used
> instead
>>       of having to create specific  handlerton::fill_*_table methods.
>>       
>>       In working on this issue a major change that is obvious is that I have
> moved the definition
>>       of the enumeration enum_schema_tables from sql/table.h to sql/handler.h.
>>       
>>       sql/table.h and sql/handler.h have a sort of cyclic dependency in between
> them, in that
>>       sql/handler.h uses structures defined in sql/table.h, while, sql/table.h
> uses enumerations
>>       defined in sql/handler.h. So to compensate for the problems that arise from
> sql/handler.h
>>       included before sql/table.h, sql/handler.h includes forward declarations of
> the structures.
>>       
>>       In our case since we need an enumeration(enum_schema_tables) from
> sql/table.h in sql/handler.h. 
>>     
>
> Try to avoid very long lines in the changeset comment, please.
>
>   
>>       This would result in problems since enumerations cannot be forward
> declared. 
>>       
>>       The problem can however be solved by moving enum_schema_tables from
> sql/table.h to sql/handler.h.
>>       
>>       This however might not be semantically appealing to some folks. 
>>       
>>       The workaround for enum forward declarations can be found here
> http://www.ddj.com/cpp/184403894. 
>>       I did not find this solution elegant atleast to our context. If this seems
> better to folks I can
>>       possibly attempt this.
>>     
>
> This last paragraph is unnecessary. Think this way - changeset comment
> goes into a branch history. Forever. Not like an email which is read and
> forgotten.  Consequently, changeset comment should only describe changes
> in this changeset. It shouldn't have anything which will be not
> interesting or relevant in, say, five years to somebody who will look
> and the branch revision history.
>
>   
>> modified:
>>   sql/ha_ndbcluster.cc
>>   sql/handler.h
>>   sql/mysql_priv.h.pp
>>   sql/sql_show.cc
>>   sql/table.h
>>
>> === modified file 'sql/handler.h'
>> --- a/sql/handler.h	2008-05-08 13:01:30 +0000
>> +++ b/sql/handler.h	2008-06-27 05:11:51 +0000
>> @@ -716,6 +759,9 @@ struct handlerton
>>     int (*fill_files_table)(handlerton *hton, THD *thd,
>>                             TABLE_LIST *tables,
>>                             class Item *cond);
>>     
>
> you forgot to remove fill_files_table()
>
>   
>> +   int (*fill_is_table)(handlerton *hton, THD *thd, TABLE_LIST *tables, 
>> +                        class Item *cond, 
>> +                        enum enum_schema_tables);
>>     uint32 flags;                                /* global handler flags */
>>     /*
>>        Those handlerton functions below are properly initialized at handler
>>
>> === modified file 'sql/mysql_priv.h.pp'
>> --- a/sql/mysql_priv.h.pp	2008-06-17 17:17:25 +0000
>> +++ b/sql/mysql_priv.h.pp	2008-06-27 05:11:51 +0000
>> @@ -4510,35 +4551,35 @@ public:
>>    int ha_index_init(uint idx, In_C_you_should_use_my_bool_instead() sorted)
>>    {
>>      int result;
>> -    const char *_db_func_, *_db_file_; uint _db_level_; char **_db_framep_;
> _db_enter_
> ("ha_index_init","./sql/handler.h",1417,&_db_func_,&_db_file_,&_db_level_,
> &_db_framep_);
>> +    const char *_db_func_, *_db_file_; uint _db_level_; char **_db_framep_;
> _db_enter_
> ("ha_index_init","./sql/handler.h",1463,&_db_func_,&_db_file_,&_db_level_,
> &_db_framep_);
>>     
>
> Oops. We need to remove this from handler.h.
> And other implementation methods too, see below in the .pp file.
> As a part of WL#4394, if it'll be done soon, or WL#4380 if not.
> mysql_priv.h.pp is useless until we do that :(
> Note - fixing this is *not* part of WL#4448.
>
> Regards / Mit vielen Grüssen,
> Sergei
>
>