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
