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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp