Mats Kindahl wrote:
> Hi Rafal!
>
> I have looked at the code, and it appears to be correct, albeit hard to
> follow. I have some suggestions to improve the overall structure of the
> code and make it easier to maintain.
>
> In general, the principle of using a mixin (they are called that) is
> sound, but there are a few guidelines to following when using a mixin.
> They are not written in stone, but one should not deviate from them
> unless there is a good reason:
I think there is a good reason for deviating from "no virtual functions in
mixin" guideline. Here it goes:
The main purpose of introducing my mixin Rows_log_event_old, is to move here old
implementation of do_apply_event() method to be used by derived
{Write,Update,Delete}_rows_log_event_old classes. This old implementation uses
do_xxxx() primitives, for example do_exec_row().
If I want to use the old code as implementation of
Rows_log_event_old::do_apply_event(), I must declare do_exec_row() and other
primitives in this class. And I must declare them as virtual since they will be
defined only in the derived classes {Write,Update,Delete}_rows_log_event_old.
Lets look at my example again:
1) Write_rows_log_event_old ev;
2) ev->do_apply_event(...)
3) Rows_log_event_old::do_apply_event(ev,...)
4) ev->do_exec_row(...) // this will call version defined in
// Write_rows_log_event_old
In line 4, correct implementation will be called only if do_exec_row(...) is
virtual. If it were not, then it would have been defined inside
Rows_log_event_old, but this is not possible since implementation depends on the
type of event.
>> Rows_log_event_old
>> /
>> Write_rows_log_event_old
>> \
>> Write_rows_log_event - Rows_log_event
>>
>>
>> But do_apply_event() operates on instances of Rows_log_event class.
>> Therefore I must pass pointer to Rows_log_event instance to the method
>> and make Rows_log_event_old a friend of Rows_log_event so that it can
>> access all members.
>> This leads to the following solutions:
>>
>> Rows_log_event_old::do_apply_event(Rows_log_event *,...)
>>
>> Write_rows_log_event_old::do_apply_event(...)
>> {
>> Rows_log_event_old::do_apply_event(this,...)
>> }
>>
>
> Since this is not the same as the do_apply_event() in Rows_log_event, I
> suggest changing the name.
>
Ok, I can easily change it to e.g. do_apply_event_old() and will make it
non-virtual.
>> And here we come to the most suspicious part of my code. For example
>> let's take do_exec_row() primitive redefined in
>> Write_rows_log_event_old. The class inherits this method from both
>> Rows_log_event_old and Write_rows_log_event and in both these classes
>> the method is declared virtual. But, AFAIK, this should not be a
>> problem. After redefining do_exec_row() in Write_rows_log_event_old,
>> this definition will be always used. For example:
>
> Then you don't need to have do_exec_row() in Rows_log_event_old virtual
> either. Also, since this again is not the same as do_exec_row() in
> Rows_log_event, I suggest you rename it to something else.
>
I think it must be virtual as I explained above. Changing its name would require
renaming all uses of it in the old code, instead of just cutting-and-pasting it
from log_event.cc to log_event_old.cc - an error prone procedure. Do you really
insist on that? In my opinion, distinction between
Write_rows_log_event::do_exec_row() and Write_rows_log_event_old::do_exec_row()
is as good as the one between do_exec_row() and do_exec_row_old().
Rafal
