Hi Andrei,
Andrei Elkin, 13.11.2008 14:23:
...
> I have started today with wondering about the new class
>
> sys_var_const
>
> Now I find it is better be based on
>
> sys_var_readonly
>
> instead.
>
> Perhaps you see the idea,
>
>
>> +class sys_var_const: public sys_var
>> +{
>> +public:
>> + enum_var_type var_type;
>> + SHOW_TYPE show_type_value;
>> + uchar *ptr;
>> + sys_var_const(sys_var_chain *chain, const char *name_arg, enum_var_type type,
>> + SHOW_TYPE show_type_arg, uchar *ptr_arg)
>> + :sys_var(name_arg), var_type(type),
>> + show_type_value(show_type_arg), ptr(ptr_arg)
>> + { chain_sys_var(chain); }
>> + bool update(THD *thd, set_var *var) { return 1; }
>> + bool check_default(enum_var_type type) { return 1; }
>> + bool check_type(enum_var_type type) { return type != var_type; }
>> + bool check_update_type(Item_result type) { return 1; }
>> + uchar *value_ptr(THD *thd, enum_var_type type, LEX_STRING *base)
>> + {
>> + return ptr;
>> + }
>> + SHOW_TYPE show_type() { return show_type_value; }
>> + bool is_readonly() const { return 1; }
>> +};
>>
>
> I suggest
>
> 1. to s/sys_var/sys_var_readonly/;
> 2. to add a new member id function that returns its argument
> so that *value_ptr will call it. It should be "shared" member of
> the new class;
> 3. to modify constructor correspondingly, i.e
> to assign sys_var_readonly::value_ptr_func to be the id function of
> the new sys_var_const class
>
> and that's it.
I always suspected that there must be some secret behind MySQL
development. Reading MySQL source code I frequently sense it could be:
"Make the code complicated and difficult to read for others". But I have
not been told so explicitly yet.
So what's wrong with the above quoted class? It is clean, simple, and
easy to understand. The result of your suggestion of a "shared" "member
id function" is surely more difficult to understand. If only, that one
has to read and understand two classes.
But what is the goal at all? To base one class on the other as you said?
Why would that be desirable? Usually one inherits to reuse code. But
here we have a couple of empty member functions, just returning a
simple, mostly constant value. Reusing these for the price of more
difficult to understand code raises my contradiction. Since other
sys_var classes don't show the highest possible inheritance either, I
guess you may have something else in mind. Can you please explain?
Last not least another small thought. Either I don't understand, what
you plan, or it might not work. I understand your suggestion so that
sys_var_const shall supply a function for sys_var_readonly. That
function shall return its argument. Since sys_var_readonly calls the
function with 'thd' as its only argument, sys_var_const::value_ptr()
would return 'thd', where it should return the pointer that it received
with the constructor.
I would prefer to keep sys_var_cont apart from sys_var_readonly. The
latter is used for variables that change their value over time (e.g.
error_count), while the former is for constant values.
>
>
> 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 / ?
> Could not that make the new branch in Item *sys_var::item() unnecessary?
I don't believe so. As I mentioned above, sys_var_readonly calls the
value_ptr_func with a 'thd' argument only. We would need to rewrite
sys_slave_skip_errors() so that it can live without the 'var' and 'buff'
parameters. Too much change for my taste. If it is possible at all.
>
> Please find me on #rep,
>
> cheers,
>
> Andrei
Can you please confirm that your writing ended here, and that the
remaining 90% of the email, which is did not quote, were just a quote of
my commit email?
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
