Hi Bar,
Thanks for the new patch. I think the changes you have made are good.
I will send some details comments in reply to the commit mail. The
main issues I have now is:
- fix_charset(), see below for a discussion
- Testing: Why are all the tests based on concat? I would have
expected to see tests for all the functions that are listed in the
"Proofs" section of the worklog. Some are tested, but wrapped in
concat. Some, I do not find tests for at all. And are all tests
for concat really necessary? What difference do expect to find
between concat(acos(0.5)) and concat(asin(0.5)). Both basically
tests concat of a double.
Alexander Barkov wrote:
...
> Note, there are now a lot of pieces of code like this:
>
> max_length= 6;
> fix_charset();
>
>
> I still suggest to introduce a new method:
>
> fix_length_and_charset(6);
>
> which will set both length and charset in the simple cases like this.
> I.e. the same action what set_char_length() did in the previous version,
> but with a better name :)
I agree that the above is not very good. I do not think the major
problem is the repetition of code itself, but the following:
1. The above code assumes that before fix_charset() is called, the
current charset has mbmaxlen==1. Even if that is always the case
today, it would be good if such dependencies could be avoided.
This is kind of the same objection that I had earlier with
the unit of max_length changing from #chars to #bytes in the
middle of the code.
2. This style kind of highlights the default case. That is,
fix_charset() is only called when the default charset is to be
used. Expections from this rule, is often less visible (since it
sometimes is achieved by not calling fix_charset). I think it
would be good if the exceptions from the rule were more explicit.
I guess the reason you cannot set charset in the constructor is that
the default charset may change before you do fix_fields. Am I right?
I wonder whether the best would be if fix_fields set charset to
default_charset in the beginning, and then those classes that wanted
something else could set that explicitly in its fix_length_and_dec().
(This could be done by having a non-virtual "fix_fields" that did this
before calling the existing virtual fix_fields, which had to be
renamed.) Then, max_length would always represent #bytes, and would
have to be set through a set_char_length method that always to charset
into account.
Alternatively, I am OK with a fix_length_and_charset, but I think it
would be best if charset is explicit
fix_length_and_charset(6, default_charset());
and that fix_length_and_charset should always be used. That is, no
direct setting of max_length and charset. Would that work?
I think fix_length_and_charset would then look something like this:
void fix_length_and_charset(uint32 char_length, CHARSET_INFO *cs)
{
max_length= length * cs->mbmaxlen;
collation.collation= cs;
}
That is, no dependence on the previous setting.
What do you think?
--
Øystein
