maria-developers team mailing list archive

[Yuchen Pei <yuchen.pei@xxxxxxxxxxx>]

Hi,

Thanks for the comments. I have addressed them and updated the jira
comment[1] with the urls to the new patch.

https://jira.mariadb.org/browse/MDEV-28152?focusedCommentId=253836&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-253836

On Mon 2023-03-20 12:01:49 +0200, Michael Widenius wrote:

> Hi!
>
> I couple of small comments in addition to Sanjas review:
>
>> > +longlong sequence_definition::truncate_value(const Longlong_hybrid& original)
>> > +{
>> > +  if (is_unsigned)
>> > +    return original.to_ulonglong(value_type_max());
>> > +  else if (original.is_unsigned_outside_of_signed_range())
>> > +    return value_type_max();
>> > +  else
>> > +    return original.value() > value_type_max() ? value_type_max()
>> > +      : original.value() < value_type_min() ? value_type_min()
>> > +      : original.value();
>
>   Please remove all 'else' above.
>   not needed and makes lines shorter.
>
>   Please cache also original.value() in a local variable.
>   Yes, it's a currently defined as { return m_value; } but as this may change
>   in the future, it's better to be sure and store in a variable.
>   Doing that will also make the code text sligtly shorter:
>
>   Please also adjust the indentention to be as follows:
>
>    return (value > value_type_max() ? value_type_max() :
>           value < value_type_min() ? value_type_min()
>           value);
> In other words
> - Operators last on the line
> - Long operations should be surronded by braces to make it easier for the editor
>   to align things properly.

Done.

>
>
>> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
>> > index e3298a4a6c1..27ce8bcdbfc 100644
>> > --- a/sql/sql_yacc.yy
>> > +++ b/sql/sql_yacc.yy
>> > @@ -2605,13 +2611,25 @@ sequence_defs:
>> >          ;
>> >
>> >  sequence_def:
>> > -          MINVALUE_SYM opt_equal sequence_truncated_value_num
>> > +          AS int_type field_options
>> > +          {
>
> Please create a local variable:
>                  sequence_definition *seq= lex->create_info.seq_create_info;
> and use it below. Makes the long lines much shorter!
>
>> > +            if (unlikely(Lex->create_info.seq_create_info->used_fields &
>> > +                         seq_field_used_as))
>> > +              my_yyabort_error((ER_DUP_ARGUMENT, MYF(0), "AS"));
>> > +            if ($3 & ZEROFILL_FLAG)
>> > + my_yyabort_error((ER_BAD_OPTION_VALUE, MYF(0), "ZEROFILL",
>> > "AS"));
>> > +            Lex->create_info.seq_create_info->value_type = $2->field_type();
>> > + Lex->create_info.seq_create_info->is_unsigned = $3 &
>> > UNSIGNED_FLAG ? true : false;
>> > + Lex->create_info.seq_create_info->used_fields|=
>> > seq_field_used_as;
>> > +          }

Done.

>
> Regards,
> Monty


Best,
Yuchen