Hi Bar,
I am sorry that I did not discover your new patch before now. There
seems to have been something wrong with my commit-list filters. I
will take a look at your new patch immediately. I have read your
comments belowto my latest review and your responses looks fine to me.
I only have a few in-line comments on the stuff related to
mysql_priv.h. See below.
Thanks,
--
Øystein
Alexander Barkov wrote:
...
>> > === modified file 'sql/mysql_priv.h'
>> > --- a/sql/mysql_priv.h 2009-08-14 19:52:00 +0000
>> > +++ b/sql/mysql_priv.h 2009-08-20 10:18:05 +0000
>>
>> Note that there is ongoing work by Mats to split file. If possibly,
>> you should try to avoid to add new stuff to this file.
>
> Ok.
>
I am not sure what will happen to the work Mats was doing to split
mysql_priv.h. My comments might not be that relevant anymore.
>>
>>
>> > @@ -175,7 +175,8 @@ extern CHARSET_INFO *error_message_chars
>> >
>> > enum Derivation
>> > {
>> > - DERIVATION_IGNORABLE= 5,
>> > + DERIVATION_IGNORABLE= 6,
>> > + DERIVATION_NUMERIC= 5,
>> > DERIVATION_COERCIBLE= 4,
>> > DERIVATION_SYSCONST= 3,
>> > DERIVATION_IMPLICIT= 2,
>> > @@ -183,6 +184,8 @@ enum Derivation
>> > DERIVATION_EXPLICIT= 0
>> > };
>> >
>> > +#define my_charset_numeric my_charset_latin1
>> > +#define MY_REPERTOIRE_NUMERIC MY_REPERTOIRE_ASCII
>>
>> Could these defines go somewhere else?
>
> Not sure. It could go to m_ctype.h.
> But these defines are not needed on the client side.
> So mysql_priv.h looks the best place.
But if mysql_priv.h is to go away, it must be moved somewhere later.
Where do you suggest?
>
>
>>
>> >
>> > typedef struct my_locale_errmsgs
>> > {
>> > @@ -827,6 +830,16 @@ typedef Comp_creator* (*chooser_compare_
>> > #include "item.h"
>> > extern my_decimal decimal_zero;
>> >
>> > +/* my_decimal.cc */
>> > +bool str_set_decimal(uint mask, const my_decimal *val, uint
>> fixed_prec,
>> > + uint fixed_dec, char filler, String *str,
>> > + CHARSET_INFO *cs);
>> > +inline bool str_set_decimal(const my_decimal *val, String *str,
>> > + CHARSET_INFO *cs)
>> > +{
>> > + return str_set_decimal(E_DEC_FATAL_ERROR, val, 0, 0, 0, str, cs);
>> > +}
>> > +
>>
>> This method seems to be only used in one place. Do you really need to
>> make it globally available?
>
> Sorry, which one? str_set_decimal() is quite big.
> my_decimal.cc looks the best place for it.
>
I think my point was that since this new str_set_decimal() is only
used one file, it seems a bit of an overkill to make it globally
available through declaring it in mysql_priv.h. Would it not be
better to include the declaration only where it is used?
