Ingo, good day.
> 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.
Sorry, let's state it more precise. It's not "just to ignore". You are
not mentioning other reasons - avoiding duplication and the
future-change-proof - ideas you must have credited.
This example
>
> 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.
should not hold any magic. It'd be the right thing to do fixing the definition
the way that you are suggesting.
Not to call that relating to the current work though.
> 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.
>
I appreciate your reluctance. However, I doubt there is any philosophy in
not doing at the first place sys_var_const (sys_var_readonly)
but one of the least immediate efforts :-)
> ...
>>>> 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.
Pondering about this 2nd change, I find it even more appropriate. A new class
keeps its logics isolated so that we would not need two
extensions (I thought that was only one in sys_var::item at first) in
a general methods: bool show_status_array() and sys_var::item()
And I am also curious what would Mark say.
cheers,
Andrei
