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