List:Internals« Previous MessageNext Message »
From:Jay Pipes Date:April 13 2009 1:33pm
Subject:Re: adding a member to classes "THD" and "handler": conditionally or
not?
View as plain text  
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Guilhem Bichot wrote:
> Hi Jay,
> 
> Jay Pipes a écrit, Le 11.04.2009 16:11:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Guilhem Bichot wrote:
>>> Hello Serg,
>>>
>>> doing a review of Marc's P_S patch, I'm seeing two members "m_psi" added
>>> into those two classes; they are currently added unconditionally even
>>> though they are used only if P_S is compiled in (which is determined by
>>> the definition of HAVE_PSI_INTERFACE). Like this in handler.h:
>>> @@ -1513,6 +1519,8 @@
>>>    */
>>>    uint auto_inc_intervals_count;
>>>
>>> +  PSI_table *m_psi;
>>>
>>> I am going to suggest that the members should be added _conditionally_
>>> instead (inside #ifdef HAVE_PSI_INTERFACE #endif). Because for me, when
>>> a feature is not compiled in, most of its traces should go away; no
>>> reason to grow classes by a few bytes for nothing.
>>> But, what is the policy nowadays, given that "handler" is a public
>>> interface, where we aim for plugins compiled with options X to work with
>>> server compiled with option Y, ABI compatibility etc? Is the policy to
>>> #ifdef or not #ifdef in this class?
>>> And I have the same question about THD which some engines access.
>>> Thanks!
>>
>> I know it's probably too big of a change for main MySQL, but when stuff
>> like this pops up, it's generally better solved via object-orientation.
>>
>> i.e. You base the Handler class and subclass a specialized one which
>> would be used for the PSI interface, allowing a base class pointer to be
>> passed in the interface, and the PSI module to
>> static_cast<SomeDerivedPSIHandler *> the passed in base class pointer...
> 
> Uh, you lost me (I promise I read the paragraph 3 times). Could you
> please sketch some pseudo-code?

Hi Guilhem! Sorry about that; I agree, it wasn't a very good
explanation! :) What I meant was that when I see code that has a class
with some parts of it #ifdef'd out, like so:

class Foo
{
protected:
  int my_int;
#ifdef some_special_feature
  int my_special_int;
#endif
}

I believe that it should usually be two classes, like so:

class Foo
{
protected:
  int my_int;
public:
  void do_something();
}

class SpecialFoo :public Foo
{
protected:
  int my_special_int;
public:
  void do_something(); /* Override for the special case... */
}

That way, depending on whether a Foo or a SpecialFoo is constructed,
either the base or the overridden function would get called...

Cheers,

Jay
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAknjPxcACgkQ2upbWsB4UtHezQCfV6H50i5kuCIwCp8eGJ1CVYtU
pIwAn0VXU2ohxRdAFf3UwUiU2UlIfKF8
=RnGK
-----END PGP SIGNATURE-----
Thread
adding a member to classes "THD" and "handler": conditionally or not?Guilhem Bichot11 Apr
  • Re: adding a member to classes "THD" and "handler": conditionallyor not?Sergei Golubchik11 Apr
    • Re: adding a member to classes "THD" and "handler": conditionally ornot?Guilhem Bichot12 Apr
  • Re: adding a member to classes "THD" and "handler": conditionally ornot?Jay Pipes11 Apr
    • Re: adding a member to classes "THD" and "handler": conditionally ornot?Guilhem Bichot12 Apr
      • Re: adding a member to classes "THD" and "handler": conditionally ornot?Jay Pipes13 Apr
    • Re: adding a member to classes "THD" and "handler": conditionally ornot?Mats Kindahl14 Apr