Hello Alexander, Thanks for your great review. Regards, Jérôme.
> -----Message d'origine----- > De : Alexander Barkov [mailto:b...@mariadb.org] > Envoyé : dimanche 5 février 2017 14:45 > À : jerome brauge; MariaDB Developers (maria- > develop...@lists.launchpad.net) > 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...) > - 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 I add these tests to the suite. > - 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. 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. 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!
0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch
Description: 0001-MDEV-10697-sql_mode-ORACLE-GOTO-statement.patch
_______________________________________________ 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
