Hi Andrei,
thank you for your sensible comments. I do now feel much better with
your suggestions. But still... Please see below.
Andrei Elkin, 14.11.2008 16:16:
> Hello, Ingo!
>
>>> Perhaps you see the idea,
>>>
>>>
>>>> +class sys_var_const: public sys_var
...
> How do you find the 2nd definition?
I like it better as it won't add complexity. But then it inherits from a
class, just to ignore a vital member it introduces.
Looking at other sys_var_* classes, I notice that it is quite common
that classes inherit from sys_var[_thd] instead of from similar classes.
Please examine these:
class sys_var_const_str :public sys_var
class sys_var_const_str_ptr :public sys_var
class sys_var_enum_const :public sys_var
class sys_var_readonly: public sys_var
If we would want to have a model of inheritance with low duplication, we
probably would have something like:
class sys_var_nowrite :public sys_var
class sys_var_readonly: public sys_var_nowrite
class sys_var_const :public sys_var_nowrite
class sys_var_const_str :public sys_var_const
class sys_var_const_str_ptr :public sys_var_const
class sys_var_enum_const :public sys_var_const
So I conclude that there must be some magic, that we don't see. I'm
reluctant to break the sys_var philosophy. I would rather see the
opinion of the second reviewer. If he agrees with you then I'll follow.
...
>>> The second issue,
>>>
>>>> +static sys_var_const sys_slave_skip_errors(&vars,
> "slave_skip_errors",
>>>> + OPT_GLOBAL, SHOW_FUNC,
>>>> + (uchar*)
> show_slave_skip_errors)
>>> What if for sys_slave_skip_errors() we would choose the parent
>>> sys_var_readonly class and s/ SHOW_FUNC / SHOW_CHAR / ?
...
> But again, if that's the whole deal, then yet another class can be
> defined
Ok. If I'm outvoted and have to rework the patch, I'll try that change.
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
