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:b...@mariadb.org] > 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: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? 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!
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
