maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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@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

Thanks! Addressed your suggestions and pushed.

> 
> Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>