Hi Sergei, On 01/23/2017 07:50 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Jan 11, Alexander Barkov wrote:
commit adf6bb91e349e8983b146cbfcaaa002edff54fe5 Author: Alexander Barkov <bar@mariadb.org> 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
Thanks! Addressed your suggestions and pushed.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org