maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hello Jerome,

The last version looks very good. Thanks!.

Note, I haven't made a detailed review for sp-goto.test yet.
Will reply about tests in a separate letter.


On 02/08/2017 06:31 PM, jerome brauge wrote:
> Hi Alexander,
> I'm sorry to waste your time for formatting and/or coding style issues.
> Is there a document that lists your best practices ?

No problem. Generally, we use MySQL coding style:

https://dev.mysql.com/doc/internals/en/general-development-guidelines.html

Please find coding style suggestions in the attached file.

Also, please find comments inline:


<cut>

>>
>> Sorry, I might be not clear enough.
>>
>> Please rename "sp_non_labelable_stmt" to "sp_labelable_stmt".
>> My idea of this name was that these statements are ALLOWED to have a
>> label, they can appear either with a label or without a label.
>>
>> Btw, are there any statements in PL/SQL that cannot have a label?
> 
> Done. 
> 
> The only place where Oracle doesn't accept a label is just before an end of block.

Thanks. So my original assumption about two separate rules for
labelable and non-labelable statements was wrong. All statements
are labelable.
I'm not an Oracle expert yet, but gradually learning it :)

So we don't need sp_non_labelable_stmt them. Ok.

<cut>


Can you please also have a look into:

https://mariadb.com/kb/en/mariadb/mariadb-contributor-agreement-frequently-asked-questions/

We require contributors to share the code either under terms of MCA
or BSD-new licenses.



Also, when we finish reviews, would it be possible to ask you
to make a git pull request?

We prefer to merge pull requests instead of pushing the patch on behalf
of the contributor.

Thanks!

> From e26a84946712e346d43f2eaa5c9d7ec86500910a Mon Sep 17 00:00:00 2001
> From: jb <jb@JB-PC>
> Date: Mon, 6 Feb 2017 10:13:35 +0100
> Subject: [PATCH] MDEV-10697  sql_mode=ORACLE: GOTO statement
> 
> ---
>  mysql-test/suite/compat/oracle/r/sp-goto.result | 830 ++++++++++++++++++++++
>  mysql-test/suite/compat/oracle/t/sp-goto.test   | 868 ++++++++++++++++++++++++
>  sql/lex.h                                       |   1 +
>  sql/sp_head.cc                                  | 131 +++-
>  sql/sp_head.h                                   |  31 +
>  sql/sp_pcontext.cc                              |  62 +-
>  sql/sp_pcontext.h                               |  55 +-
>  sql/sql_lex.cc                                  |  48 ++
>  sql/sql_lex.h                                   |   2 +
>  sql/sql_yacc.yy                                 |   2 +
>  sql/sql_yacc_ora.yy                             |  96 ++-
>  11 files changed, 2084 insertions(+), 42 deletions(-)
>  create mode 100644 mysql-test/suite/compat/oracle/r/sp-goto.result
>  create mode 100644 mysql-test/suite/compat/oracle/t/sp-goto.test
> 
> diff --git a/mysql-test/suite/compat/oracle/r/sp-goto.result b/mysql-test/suite/compat/oracle/r/sp-goto.result
> new file mode 100644
> index 0000000..8a1bf31
> --- /dev/null
> +++ b/mysql-test/suite/compat/oracle/r/sp-goto.result


It seems you forgot to do "perl mtr --record compat/oracle.sp-goto"
after last update of sp-goto.test. This test fails for me.

Also, it seems "DROP PROCEDURE" is missing in the last test chunk.

<cut>

> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index 627b239..0caa26d 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc

<cut>

> -sp_head::push_backpatch(THD *thd, sp_instr *i, sp_label *lab)
> +sp_head::push_backpatch(THD *thd, sp_instr *i, sp_label *lab,
> +                        List<bp_t> * list, backpatch_instr_type itype)

In pointer declarations we use no spaces between * and the variable name.

  List<bp_t> *list,


<cut>


> +int
> +sp_head::push_backpatch_goto(THD *thd, sp_pcontext *ctx, sp_label *lab)
> +{
> +  uint ip= instructions();
> +
> +  // Add cpop/hpop : they will be removed or updated later if target is in
> +  // the same block or not

Multi-line comments should use /* */ style:

 /*
   Add cpop/hpop : they will be removed or updated later if target is in
   the same block or not
 */

<cut>


> +  DBUG_ENTER("sp_head::backpatch_goto");
> +  while ((bp= li++))
> +  {
> +    if ((bp->instr->m_ip < lab_begin_block->ip) || (bp->instr->m_ip > lab->ip))

   if (bp->instr->m_ip < lab_begin_block->ip || bp->instr->m_ip > lab->ip)


> +    {
> +      // update only jump target from the beginning of the block where the
> +      // label is defined.

  /*
    update only jump target from the beginning of the block where the
    label is defined.
  */

<cut>


> +sp_head::check_unresolved_goto()
> +{
> +  DBUG_ENTER("sp_head::check_unresolved_goto");
> +  bool has_unresolved_label=false;
> +  if (m_backpatch_goto.elements>0)

We use spaces around comparison operators:

 if (m_backpatch_goto.elements > 0)

<cut>


> --- a/sql/sp_head.h
> +++ b/sql/sp_head.h
> @@ -525,11 +525,19 @@ class sp_head :private Query_arena
>    DYNAMIC_ARRAY m_instr;	///< The "instructions"
> +
> +  enum backpatch_instr_type { GOTO, CPOP, HPOP };
>    typedef struct
>    {
>      sp_label *lab;
>      sp_instr *instr;
> +    uint32 instr_type;

I suggest to change the data type for inst_type from uint32
to backpatch_instr_type.

<cut>

> +  int
> +  push_backpatch(THD *thd, sp_instr *, sp_label *, List<bp_t> * list);
> +  int
> +  push_backpatch(THD *thd, sp_instr *, sp_label *, List<bp_t> * list,
> +                 backpatch_instr_type itype);

Please remove the space between * and list in the above two declarations.

<cut>

> diff --git a/sql/sp_pcontext.cc b/sql/sp_pcontext.cc
> index 06c5164..48be9b2 100644
> --- a/sql/sp_pcontext.cc
> +++ b/sql/sp_pcontext.cc
>  
> +bool cmp_labels(sp_label *a, sp_label *b)
> +{
> +  return ((my_strcasecmp(system_charset_info, a->name.str, b->name.str)==0)
> +          && (a->type==b->type));

Please add spaces around == and remove redundant paretheses:

     return my_strcasecmp(system_charset_info, a->name.str, b->name.str == 0) &&
            a->type == b->type;


> +}
> +
>  sp_pcontext *sp_pcontext::pop_context()
>  {
>    m_parent->m_max_var_index+= m_max_var_index;
> @@ -140,6 +147,18 @@ sp_pcontext *sp_pcontext::pop_context()
>    if (m_num_case_exprs > m_parent->m_num_case_exprs)
>      m_parent->m_num_case_exprs= m_num_case_exprs;
>  
> +  /*
> +  ** Push unresolved goto label to parent context
> +  */
> +  sp_label * label;

Space between * and label.

<cut>

> @@ -227,9 +246,9 @@ sp_variable *sp_pcontext::add_variable(THD *thd, LEX_STRING name)
>    return m_vars.append(p) ? NULL : p;
>  }
>  
> -
>  sp_label *sp_pcontext::push_label(THD *thd, LEX_STRING name, uint ip,
> -                                  sp_label::enum_type type)
> +                                  sp_label::enum_type type,
> +                                  List<sp_label> * list)

Space between * and list.

<cut>

> +  if (!recusive)
> +  {
> +    return NULL;
> +  }

If there is only one statement, we don't use { }.

  if (!recursive)
    return NULL;

<cut>

> +
> +  return (m_parent && (m_scope == REGULAR_SCOPE)) ?
> +         m_parent->find_goto_label(name) :
> +         NULL;

  return m_parent && m_scope == REGULAR_SCOPE ?
         m_parent->find_goto_label(name) :
         NULL;

<cut>

> diff --git a/sql/sp_pcontext.h b/sql/sp_pcontext.h
> index 00cc819..d526e2f 100644
> --- a/sql/sp_pcontext.h
> +++ b/sql/sp_pcontext.h
> @@ -94,6 +94,7 @@ class sp_variable : public Sql_alloc

<cut>


> +
> +  sp_label *sp_pcontext::push_label(THD *thd, LEX_STRING name, uint ip,
> +                                    sp_label::enum_type type)
> +  { return push_label(thd, name, ip, type, &m_labels); }
> +
> +  sp_label *sp_pcontext::push_goto_label(THD *thd, LEX_STRING name, uint ip,
> +                                         sp_label::enum_type type)
> +  { return push_label(thd, name, ip, type, &m_goto_labels); }
>  
>    sp_label *push_label(THD *thd, const LEX_STRING name, uint ip)
> -  {
> -    return push_label(thd, name, ip, sp_label::IMPLICIT);
> -  }
> +  { return push_label(thd, name, ip, sp_label::IMPLICIT); }
> +
> +  sp_label *push_goto_label(THD *thd, const LEX_STRING name, uint ip)
> +  { return push_goto_label(thd, name, ip, sp_label::GOTO); }
>  
>    sp_label *find_label(const LEX_STRING name);
>  
> +  sp_label *find_goto_label(const LEX_STRING name, bool recusive);
> +
> +  sp_label *sp_pcontext::find_goto_label(const LEX_STRING name)
> +  { return find_goto_label(name, true); }
> +
>    sp_label *find_label_current_loop_start();

Please remove all "sp_pcontext::" in the above declarations.
It does not compile with GCC.

<cut>

> +  /// List of labels for block labels
>    List<sp_label> m_labels;
> +  /// List of labels for goto labels
> +  List<sp_label> m_goto_labels;

I suggest to remove double "labels" wording in the above comments, as follows:

  /// List of block labels
  
  /// List of goto labels


>    /// Children contexts, used for destruction.
>    Dynamic_array<sp_pcontext *> m_children;
> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
> index c361ee0..6d799fb 100644
> --- a/sql/sql_lex.cc
> +++ b/sql/sql_lex.cc
> @@ -5511,6 +5511,54 @@ bool LEX::sp_leave_statement(THD *thd, const LEX_STRING label_name)
>    return sp_exit_block(thd, lab, NULL);
>  }
>  
> +bool LEX::sp_goto_statement(THD *thd, const LEX_STRING label_name)
> +{
> +  sp_label *lab= spcont->find_goto_label(label_name);
> +  if ((!lab) || (lab->ip == 0))

  if (!lab || lab->ip == 0)

> +  {
> +    sp_label * delayedlabel;

 sp_label *delayedlabel;

> +    if (!lab)
> +    {
> +      // Label not found --> add forward jump to an unknown label
> +      spcont->push_goto_label(thd, label_name, 0, sp_label::GOTO);
> +      delayedlabel= spcont->last_goto_label();
> +    }
> +    else
> +    {
> +      delayedlabel= lab;
> +    }
> +    return sphead->push_backpatch_goto(thd,spcont,delayedlabel);

Please add spaces after commas:

  return sphead->push_backpatch_goto(thd, spcont, delayedlabel);


<cut>

> +bool LEX::sp_push_goto_label(THD *thd, const LEX_STRING label_name)
> +{
> +  sp_label *lab= spcont->find_goto_label(label_name,false);

 sp_label *lab= spcont->find_goto_label(label_name, false);


> +  if (lab)
> +  {
> +    if  (lab->ip != 0)
> +    {
> +      my_error(ER_SP_LABEL_REDEFINE, MYF(0), label_name.str);
> +      return true;
> +    }
> +    lab->ip= sphead->instructions();
> +
> +    sp_label * beginblocklabel= spcont->find_label(empty_lex_str);

  sp_label *beginblocklabel= spcont->find_label(empty_lex_str);

<cut>


> diff --git a/sql/sql_yacc_ora.yy b/sql/sql_yacc_ora.yy
> index 0ff2318..0df9c3f 100644
> --- a/sql/sql_yacc_ora.yy
> +++ b/sql/sql_yacc_ora.yy

<cut>

> +  sp_proc_stmt_goto:
> +          GOTO_SYM label_ident
> +          {
> +            if (Lex->sp_goto_statement(thd, $2))
> +              MYSQL_YYABORT;
> +          }
> +        ;
> +
> +

In rule declarations we don't use spaces before "rule:".

Please remove the spaces before "sp_proc_stmt_goto:".