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
>
>
