Hi, Kristofer!
On Apr 29, Kristofer Pettersson wrote:
> Sergei Golubchik skrev:
>
>>> === modified file 'sql/sql_plugin.cc'
>>> --- a/sql/sql_plugin.cc 2009-03-16 10:37:13 +0000
>>> +++ b/sql/sql_plugin.cc 2009-04-29 12:08:45 +0000
>>> @@ -28,6 +28,7 @@
>>> #endif
>>> extern struct st_mysql_plugin *mysqld_builtins[];
>>> +extern TYPELIB global_plugin_typelib;
>> better to define the typelib here, as a static variable. What is the
>> benefit of having it in mysqld.cc ?
>
> I was just trying to conform to the existing code layout, ie put global
> variables and structures in mysqld.cc. I don't mind moving the code to
> sql_plugin.cc
Move them please. other typelibs in mysqld.cc are used in mysqld.cc,
this one isn't, so there's no need to pollute the namespace by forcing
it to be 'extern' and visible to linker and other files.
>>> + /*
>>> + Allocate temporary space for the value of the tristate.
>>> + This option will have a limited lifetime and is not used beyond
>>> + server initialization.
>>> + */
>>> + options[0].value= options[1].value= (uchar **)alloc_root(mem_root,
>>> +
>>> sizeof(my_option **));
>> all ok, but you've rendered 'enabled' useless. I'd suggest to reuse it
>> instead - that is, to write
>> options[1].value= options[1].u_max_value= *enabled;
>> as before, and fix the code so that enabled is uint**, not my_bool**,
>> and allocated appropriately (sizeof(uint) bytes, not 1).
>
> Could you please explain why you think the use of **enable is to be
> encouraged? I find it very hackish and non intuitive. I can't say off hand
> what u_max_value means, but isn't it an upper bound for the largest
> possible value for the option? What does it mean to have u_max_value for an
> ENUM type anyway?
u_max_value is the max value is specified by the user. A max value for
an option --xxx=... is specified by --maximum-xxx=...
"maximum" is a prefix recognized by my_getopt, similar to "enable",
"skip", or "loose". It's defined in special_opt_prefix[] in my_getopt.c
A very obscure feature, nobody knows about it. And I am not sure this
value is not ignored for ENUM :)
> As I've understood the code 'enable' is conceptually used as a bool with
> two states: true and false. What do we gain by changing it to a uint?
'enable' is used as a value pointer for the --pluginname option. It's
where my_getopt stores the value of --pluginname, the opt->value
pointer. For BOOL one byte was enough, but to hold a value of a ENUM it
should be int.
> As you see from my previous patch I went to some lengths to get rid of
> enable all together and I feel pretty strong about it. Could you please
> help me to see why I should reconsider this desire to blast enable to
> oblivion?
No, you don't, really. If you want to get rid of it - go ahead,
implement some other way of passing the value of --pluginname option to
the caller (which you've done) and remove 'enable' completely. Or
reconsider your desire, use 'enable', and don't allocate another storage
for opt->value.
Either way is fine, what I didn't like was half-dead 'enable' which was
kind of removed but still used somewhere, and that 'enable' didn't make
a lot of sense being half-removed. I'd prefer it to be either fully used
or fully eliminated.
Regards / Mit vielen Grüßen,
Sergei
--
__ ___ ___ ____ __
/ |/ /_ __/ __/ __ \/ / Sergei Golubchik <serg@stripped>
/ /|_/ / // /\ \/ /_/ / /__ Principal Software Engineer/Server Architect
/_/ /_/\_, /___/\___\_\___/ Sun Microsystems GmbH, HRB München 161028
<___/ Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring
