maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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.

- Can't we remove sp_declare_label_alone
  and use label_declaration_oracle directly?

- 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.

- 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.

- I did not find tests for labels and GOTO inside EXCEPTION blocks.
  Can you please check if it works and add tests?

- Please don't use dynamic_cast. We build without RTTI,
  so it won't compile on some platforms.

- Please stay under the limit of 80 characters per line.
  Some lines in the patch are longer.

- Please rename func_goto.test to sp-goto.test.
  We use the func_ prefix for tests covering
  Item_func descendants.

- What is the purpose of sp_proc_stmts1_implicit_block?
  Why can't we use sp_proc_stmts1 directly?

Thanks!