maria-developers team mailing list archive

[jerome brauge <j.brauge@xxxxxxxxxxx>]

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 ?

Regards,
Jérôme.

> -----Message d'origine-----
> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx]
> Envoyé : mercredi 8 février 2017 07:06
> À : jerome brauge; maria-developers
> Objet : Re: MDEV-10697 - bb-10.2-compatibility
> 
> Hi Jerome,
> 
> On 02/06/2017 08:44 PM, jerome brauge wrote:
> > Hello Alexander,
> > Thanks for your great review.
> 
> Thanks for addressing my suggestions.
> Please see my comments inline, and some more things in the bottom of the
> letter:
> 
> >
> > Regards,
> > Jérôme.
> >
> >> -----Message d'origine-----
> >> De : Alexander Barkov [mailto:bar@xxxxxxxxxxx] Envoyé : dimanche 5
> >> février 2017 14:45 À : jerome brauge; MariaDB Developers (maria-
> >> developers@xxxxxxxxxxxxxxxxxxx)
> >> Objet : Re: MDEV-10697 - bb-10.2-compatibility
> >>
> >> Hello Jerome!
> >>
> >> On 02/03/2017 09:46 PM, jerome brauge wrote:
> >>> Hello,
> >>>
> >>> I've wrote a patch for MDEV-10697 (goto statement for
> sql_mode=oracle).
> >>>
> >>> If someone can review it.
> >>>
> >>> Thanks.
> >>>
> >>> Best regards,
> >>>
> >>> Jérôme.
> >>>
> >>
> >>
> >> Thank you very much for working on this!
> >> You've done a great job! Tests looks very good, and you have taken
> >> care of the most difficult part - GOTOs crossing nested blocks with
> >> cursors and exceptions, the code makes sure that cpop/hpop are
> correctly done.
> >> Excellent!
> >>
> >> Note, I haven't reviewed the entire patch thoroughly yet.
> >> Will do it in the beginning of the next week.
> >> I have suggestions and questions related to sql_yacc_ora.yy at this point.
> >>
> >>
> >> - Can you please clone the latest bb-10.2-compatibility and adjust the
> patch?
> >>   It's now different. For example, there is no such thing like
> >>   sp_unlabeled_block_not_atomic any more, because MDEV-10655 has
> been
> >> pushed.
> >>
> >
> > Done.
> >
> >> - Can't we remove sp_declare_label_alone
> >>   and use label_declaration_oracle directly?
> >
> > Done.
> >
> >> - Please rename sp_labeled_stmt to sp_labelable_stmt.
> >>   "sp_labeled_stmt" misleadingly makes the reader think that this is
> >>   a statement that HAS a label. But in fact, this is a statement
> >>   that CAN have a label (but not necessarily has).
> >>
> >>   The same for sp_unlabeled_stmt: please rename it to
> >> sp_non_labelable_stmt.
> >>   As the grammar implements "a statement that CANNOT have a label"
> >>   rather than"a statement that does not have a label".
> >>   By the way, perhaps there is no a need for this rule at all.
> >>   It's used only one time. So its sub-rules can be moved
> >>   directly to sp_proc_stmt.
> >
> > Done (I agree, names was very bad...)
> 
> 
> 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.

CREATE OR REPLACE PROCEDURE f20(res OUT VARCHAR)
AS
BEGIN
  res:=1;
  <<lab>>
END;

Failed with:
PLS-00103: Encountered the symbol "END" when expecting one of the following:

   ( begin case declare exit for goto if loop mod null raise
   return select update while with <an identifier>
   <a double-quoted delimited-identifier> <a bind variable> <<
   continue close current delete fetch lock insert open rollback
   savepoint set sql execute commit forall merge pipe purge

We have the same behavior.

> 
> >> - Why there is a need for two separate label lists
> >>   sp_pcontext::m_labels and sp_pcontext::jump labels?
> >>   Can't we put all labels the same list?
> >>   Does the fact that LOOP labels and GOTO labels reside
> >>   in different lists mean that this won't compile:
> >>
> >> DROP PROCEDURE p1;
> >> CREATE PROCEDURE p1 AS
> >>   a INT := 1;
> >> BEGIN
> >>   <<lab>>
> >>   FOR i IN a..10 LOOP
> >>     IF i = 5 THEN
> >>       a:= a+1;
> >>       GOTO lab;
> >>     END IF;
> >>   END LOOP;
> >> END;
> >> /
> >>
> >> The above works fine in Oracle.
> >> So we should try to support this.
> >
> > In fact your example already works.
> >
> > At the beginning, I was afraid of breaking your logic (push/pop of label out
> of order).
> > But your example illustrate very well the need of two list : label
> > "lab" has two meanings here
> >  - GOTO lab : must go before the beginning of the loop
> >  - CONTINUE lab : must go to the beginning of the loop
> >
> 
> Thanks for the explanation. Sounds reasonable to have separate lists.
> Can you please add a comment near m_jump_labels?
> Something like this:
> 
> /*
>   In the below example the label <<lab>> has two meanings:
>   - GOTO lab : must go before the beginning of the loop
>   - CONTINUE lab : must go to the beginning of the loop
>   We solve this by storing block labels and goto labels into separate lists.
> 
>   BEGIN
>     <<lab>>
>     FOR i IN a..10 LOOP
>       ...
>       GOTO lab;
>       ...
>       CONTINUE lab;
>       ...
>     END LOOP;
>   END;
> */
> 
> 
> > I add these tests to the suite.
> 
> Excellent.
> 
> >
> >> - I did not find tests for labels and GOTO inside EXCEPTION blocks.
> >>   Can you please check if it works and add tests?
> >
> > Done.
> >
> >> - Please don't use dynamic_cast. We build without RTTI,
> >>   so it won't compile on some platforms.
> 
> There is still one dynamic_cast left in sp_head.cc.
> sp_head::check_unresolved_jump().
> 
> >
> > Done.
> >
> >> - Please stay under the limit of 80 characters per line.
> >>   Some lines in the patch are longer.
> >
> > Done.
> >
> >> - Please rename func_goto.test to sp-goto.test.
> >>   We use the func_ prefix for tests covering
> >>   Item_func descendants.
> >
> > Done.
> >
> >> - What is the purpose of sp_proc_stmts1_implicit_block?
> >>   Why can't we use sp_proc_stmts1 directly?
> >
> > I need to have a new sp_pcontext even if I haven't an explicit bloc (begin -
> end) to isolate the scope of label definition.
> > It's the case for IF/ELSE, CASE/WHEN and probably for EXCEPTION/WHEN.
> > At this time we must add a begin/end in the when clause of exception
> handler if we have several instruction.
> 
> Thanks for clarification!
> 
> > CREATE or replace procedure p1()
> > AS
> >   a INT;
> > BEGIN
> >   begin
> >     SELECT a INTO a FROM information_schema.tables;
> >   EXCEPTION
> >     WHEN TOO_MANY_ROWS THEN
> >            a:=10;
> >            goto lab;
> >     WHEN NO_DATA_FOUND THEN
> >            a:=-5;
> >   end;
> >   <<lab>>
> >   return ;
> > END;
> >
> > The above works fine in Oracle.
> >
> >> Thanks!
> >
> 
> Here are some more suggestions/questions:
> 
> 
> - It seems the patch has some redundant trailing spaces
>   and new lines. Please do "git diff --check" before doing
>   "git commit".
> 
> - Please use two spaces for indentation.
>   In some cases you use four and three spaces, e.g. in
>   sp_head::push_backpatch_jump(), sp_head::check_unresolved_jump(),
>   LEX::sp_goto_statement() (before "delayedlabel= lab;")
> 

Done.

> - In conditions we try not to use redundant parentheses:
> 
>     if ((m_scope == HANDLER_SCOPE)&&(m_parent))
> 
>   We usually write like this (it's easier to read):
>     if (m_scope == HANDLER_SCOPE && m_parent)
> 
> - Please use static_cast, e.g.:
>     static_cast<sp_inst_cpop*>(bp->instr)
>   instead of C-style cast"
>     ((sp_instr_cpop *)(bp->instr))
> 
>   static_cast is safer. In case of possible future refactoring in the
>   class hierarchy, static_cast will catch pointer incompatibility at
>   compilation time, while C-style cast will just silently fallback to
>   reinterpret_cast.

Thanks for explanation. I'm a newbie in CPP (I know much more about pure C and java)

> 
> - It seems sp_head::check_unresolved_jump() should return a bool result:
>   true on failure and false on success, and the calling code in
>   sql_yacc_ora.yy should abort if it fails:
>     if (sp->check_unresolved_jump())
>       MYSQL_YYABORT;

Done (I had forgot a dynamic cast also)


> - I suggest to rename "jump" to "goto" in all new members and methods,
>   e.g. m_jump_labels, backpatch_jump(), check_unresolved_jump(),
>   find_jump_label(), JUMP in the enum backpatch_instr_type,
>   and so on.

Done.

> - In tests please use error names rather than numeric values,
>   e.g. "--error ER_SP_LILABEL_MISMATCH" instead of "--error 1308".
>   See mysqld_ername.h for name definitions.
> 

Done.

> 
> - The grammar does not allow labels before labels.
>   The below examples return a syntax error:
> 
> DROP PROCEDURE p1;
> DELIMITER $$
> CREATE PROCEDURE p1 AS
> BEGIN
>   <<lab1>>
>   <<lab2>>
>   NULL;
> END;
> $$
> DELIMITER ;
> 
> 
> DROP PROCEDURE p1;
> DELIMITER $$
> CREATE PROCEDURE p1 AS
> BEGIN
>   <<lab1>>
>   <<lab2>>
>   BEGIN
>     NULL;
>   END;
> END;
> $$
> DELIMITER ;
> 
> 
> DROP PROCEDURE p1;
> DELIMITER $$
> CREATE PROCEDURE p1 AS
> BEGIN
>   <<lab1>>
>   <<lab2>>
>   FOR i IN 1..10 LOOP
>     NULL;
>   END LOOP;
> END;
> $$
> DELIMITER ;
> 
>   Similar procedures work fine in Oracle.

Done. Tests has been added to the suite.

> 
> Thanks!

Attachment: 0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch
Description: 0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch