maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Jan 11, Alexander Barkov wrote:
> commit adf6bb91e349e8983b146cbfcaaa002edff54fe5
> Author: Alexander Barkov <bar@xxxxxxxxxxx>
> Date:   Wed Jan 11 13:42:52 2017 +0400
> 
>     MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
>     
>     Problem: Item_param::basic_const_item() returned true when fixed==false.
>     This unexpected combination made Item::const_charset_converter() crash
>     on asserts.
>     
>     Fix:
>     - Changing all Item_param::set_xxx() to set "fixed" to true.
>       This fixes the problem.
>     - Additionally, changing all Item_param::set_xxx() to set
>       Item_param::item_type, to avoid duplicate code, and for consistency,
>       to make the code symmetric between different constant types.
>       Before this patch only set_null() set item_type.
>     - Moving Item_param::state and Item_param::item_type from public to private,
>       to make sure easier that these members are in sync with "fixed" and to
>       each other.
>     - Adding a new argument "unsigned_arg" to Item::set_decimal(),
>       and reusing it in two places instead of duplicate code.
>     - Adding a new method Item_param::fix_temporal() and reusing it in two places.
>     - Adding methods is_no_value(), is_long_data_value(), is_int_value(),
>       instead of direct access to Item_param::state.

Looks ok to push.

Stylistic comments:
 * too many fixed= true in all set_* methods. I'd rather added a method
   fix_type(), to be used like

-     type= Item::STRING_ITEM
+     fix_type(Item::STRING_ITEM)

and in this method I'd { type=type_arg; fixed= true }.

 * names like is_no_value/is_int_value/etc look strange, I'd
   use has_no_value, has_int_value, etc

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx