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!