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: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...) 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? >> - 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;") - 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. - 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; - 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. - 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. - 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. Thanks! _______________________________________________ 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
