Alexander Barkov wrote:
> Hi Øystein,
>
> Alexander Barkov wrote:
>
> <skip>
>>> >
>>> >
>>> > @@ -324,7 +327,7 @@ public:
>>> > double val_real();
>>> > String *val_str(String*str);
>>> > enum Item_result result_type () const { return INT_RESULT; }
>>> > - void fix_length_and_dec() {}
>>> > + void fix_length_and_dec() { set_char_length_and_dec(21); }
>>> > };
>>> >
>>>
>>> Why do you need to set this when it is set in the constructor of the
>>> parent class?
>>>
>>> By the way, lot's of fix_length_and_dec functions uses hard-coded
>>> constants. Why not use the constructor instead?
>>
>> Thank for noticing this. This change is not needed. Removed.
>
> Oops. Tests started to fail after reverting this change.
> Without this line, charset is not set to @@character_set_connection,
> so it returns BINARY(21) instead of VARCHAR(21).
>
> We do need this call. But the methods names are really confusing.
> They also set charset/collation.
>
> Hmm. Should I rename the methods to:
>
> set_charset_and_char_length()
>
> and
>
> set_charset_and_char_length_and_dec()
>
> ?
>
> Sounds cumbersome :(
Just an idea: How about setting a default charset and length at
construction time, and introduce a method set_charset() that will modify
max_length accordingly if mbmaxlen for new charset is different from
mbmaxlen for old charset? I would then also remove the setting of
charset from set_char_length, and rather call both set_charset() and
set_char_length() when necessary. Makes things a bit more explicit and
easier to understand.
--
Øystein
