maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Dec 16, Alexander Barkov wrote:
> Hi Sergei,
> 
> Please review a patch for MDEV-8844.
> 
> Thanks!

> diff --git a/sql/sql_error.cc b/sql/sql_error.cc
> index b72d642..1ed3547 100644
> --- a/sql/sql_error.cc
> +++ b/sql/sql_error.cc
> @@ -931,7 +931,7 @@ char *err_conv(char *buff, uint to_length, const char *from,
>    else
>    {
>      uint errors;
> -    res= copy_and_convert(to, to_length, system_charset_info,
> +    res= copy_and_convert(to, to_length, &my_charset_errmsg,
>                            from, from_length, from_cs, &errors);

I'm not sure I like this approach. True, when all you have is a hammer -
everything looks like a nail. But we don't do *all* string conversions
with charsets, do we? For example, escaping/unescaping is done with an
explicit loop. I don't think my_charset_errmsg qualifies is a *charset*,
it's only a helper to do map unprintable characters, it's even less
charset than filename.

So I'd prefer a loop here, instead of copy_and_convert. Or, if you want
to use CHARSET_INFO, then, at least

* make it hidden and not user-selectable
* all properties and methods not directly related to err_conv() call
  should be NULL - my_charset_errmsg should not be used anywhere else.

And still, I think I'd prefer an explicit loop here.

Regards,
Sergei