Hi again!
I forgot to attach the code.
Just my few cents,
Mats Kindahl
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:
>
> * A mixin is used with MI, so do not use a common base class. This
> will lead to two instances of the base class within the class, and
> can be handled by proper use of virtual inheritance, but I
> strongly advice to stay away from that. Here be dragons!
> * There shall be no virtual functions in the mixin. The reason for
> this is because the base address for the object will be different
> in the base classes as compared to the derived class. If only one
> base class contain virtual functions, the address of the object
> will be identical in all classes.
> * Use the mixin to provide helper functions. If you are implementing
> an interface (i.e., virtual functions), define them in the derived
> class and let them refer to functions in the mixin class.
>
> As a general principle, there is no need to define a function X::fff()
> virtual unless there is a function 'tweak' somewhere with the
> signature 'tweak(X*)' or 'tweak(X&)'. Here I am ignoring any
> cv-qualifiers on the parameters, but they can be present as well.
> Also, function should here be interpreted in the wider sense, since a
> constructor serve in this case as well.
>
> If you change the code to reflect the above guidelines, it will be a
> lot easier to maintain and understand. Also, use different names for
> the mixin functions as compared to the functions that are in the
> "virtual branch" of the tree.
>
> I have attached a small C++ program to illustrate the differences
> between using virtual functions in the mixin and not using virtual
> functions.
>
> The output of the program is:
>
> mats@romeo:~/lang/cc/tests$ ./mixin
> this: 0xbfc6cccc as seen from virtual void Derived::ShowInfo() const
> this: 0xbfc6cccc as seen from void Mixin::ShowThis() const
> this: 0xbfc6ccc4 as seen from virtual void VDerived::ShowInfo() const
> this: 0xbfc6ccc8 as seen from virtual void VMixin::ShowThis() const
>
>
> Rafal Somla wrote:
>> Hi Mats,
>>
>> Thank you for your comments. Since time is pressing, I'll try to
>> explain the issues with you one by one. I think the most serious is
>> this one.
>>
>> > * The use of Old_rows_log_event does not look right. Especially
>> > since you are using multiple inheritance from two classes that
>> > both implement virtual functions. You are using a qualified name
>> > to reference to a virtual function in that class, which defeats
>> > the purpose of using a virtual function in that case. In
>> addition,
>> > you are passing the 'this' pointer to a member function
>> defined in
>> > a superclass, which doesn't look right.
>>
>> Let me explain my thinking here and let me know if I make any major
>> mistakes.
>>
>> Write_rows_log_event_old and descendants was introduced when you
>> implemented the "cannonical" row format to fix BUG#22583. Then you
>> changed type numbers for row events but kept their implementation in
>> the same classes as before (*_rows_log_event). To handle row events
>> of old types, you introduced *_rows_event_old classes which inherit
>> from the original ones. E.g.,
>>
>> class Write_rows_log_event_old: public Write_rows_log_event
>>
>> This creates a bit peculiar situation: new class which handles old
>> type of event inherits from old class which implements new type of
>> events.
>>
>> The *_rows_log_event_old classes only redefine do_prepare_row()
>> method and share (inherit) all other methods with *_rows_log_event
>> variants. This is because methods like do_apply_event(),
>> do_exec_row() and friends haven't been changed by your patch.
>>
>> Now, my changes affect do_apply_event() and do_exec_row() methods and
>> are valid only for row events of new type using the canonical format.
>> The old type events should use old versions of these methods.
>>
>> If you remember, the original structure of event applying code was as
>> follows.
>>
>> - main method is do_apply_event() and it is defined in a base of all
>> rows events: Rows_log_event class.
>>
>> - this method uses helper do_xxxx() functions which are virtual and
>> redefined in
>> leaf classes *_rows_log_event.
>>
>> Now consider, for example, Write_rows_log_event_old class which
>> inherits from Write_rows_log_event which inherits from
>> Rows_log_event. The do_apply_event() inherited from Rows_log_event is
>> modified by my patch. Thus in Write_rows_log_event_old this method
>> should be overwritten with the old implementation. The same goes for
>> {Update,Delete}_rows_log_event_old.
>>
>> Redefining do_apply_event() in each of the *_rows_log_event_old
>> classes is not an option because of code repetition. Therefore I put
>> the old implementation of do_apply_event() into a separate class
>> Rows_log_event_old and make it additional base of all
>> *_rows_log_event_old classes. In the classes I have a trivial
>> redefinition of do_apply_event() which calls the version defined in
>> Rows_log_event_old.
>>
>> Note that Rows_log_event_old doesn't inherit from Rows_log_event
>> because then we would have double inheritance like this:
>>
>> Rows_log_event_old - Rows_log_event
>> /
>> Write_rows_log_event_old
>> \
>> Write_rows_log_event - Rows_log_event
>>
>> Therefore I treat Rows_log_event as a mere container for the old
>> implementations of methods and don't put Rows_log_event as its base:
>>
>> 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.
>
>> I agree that making this method virtual makes no sense. It should not
>> be static because it uses do_xxxx() primitives declared in
>> Rows_log_event_old class and implemented in *_rows_log_event_old
>> classes.
>>
>> 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.
>
>>
>> Write_rows_log_event_old ev;
>>
>> ev->do_apply_event(...)
>> Rows_log_event_old::do_apply_event(ev,...)
>> ev->do_exec_row(...) // this will call version defined in
>> // Write_rows_log_event_old
>>
>> Note also that the implementation of do_exec_row() in
>> Write_rows_log_event_old has access to all members of
>> Write_rows_log_event as the latter class is a base for the former.
>>
>> Do you see any serious problems with this design which I have missed?
>> Do you have ideas how to do it better?
>>
>> Rafal
>>
>
>
> ------------------------------------------------------------------------
>
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
Attachment: [text/x-c++src] mixin.cc
