On Wed, Mar 22, 2017 at 7:57 PM, Ideriha, Takeshi < ideriha.take...@jp.fujitsu.com> wrote:
> Hi, thank you very much for reviewing. > Attached is v6 patch. > > >There was a minor conflict in applying 004_declareXX patch. > > I rebased 004_declareStmt_test_v6.patch. > > >Some comments in 001_declareStmt_preproc_v5.patch: > > >+ if (INFORMIX_MODE) > >+ { > >+ if (pg_strcasecmp(stmt+strlen("close "), "database") == 0) > >+ { > >+ if (connection) > >+ mmerror(PARSE_ERROR, ET_ERROR, "AT option > not allowed in CLOSE DATABASE statement"); > + > >+ fprintf(base_yyout, "{ ECPGdisconnect(__LINE__, > \"CURRENT\");"); > >+ whenever_action(2); > >+ free(stmt); > >+ break; > >+ } > >+ } > > >The same code block is present in the stmtClosePortalStmt condition to > verify the close > >statement. Because of the above code addition, the code present in > stmtClosePortalStmt > >is a dead code. So remove the code present in stmtClosePortalStmt or try > to reuse it. > > I remove almost all the stmtClosePortalStmt code > and just leave the interface which actually does nothing not to affect > other codes. > > But I'm not sure this implementation is good or not. > Maybe I should completely remove stmtClosePortalStmt code > and rename ClosePortalStmtCLOSEcursor_name to stmtClosePortalStmt to make > code easier to understand. > > >+static void > >+output_cursor_name(char *str) > > >This function needs some comments to explain the code flow for better > understanding. > > I add some explanation that output_cursor_name() filters escape sequences. > > >+/* > >+ * Translate the EXEC SQL DECLARE STATEMENT into ECPGdeclare function > >+ */ > > >How about using Transform instead of Translate? (similar references also) > > I changed two comments with the word Translate. > Thanks for the updated patch. It looks good to me. I have some comments in the doc patch. + + <para> + The third option is to declare a sql identifier linked to + the connection, for example: +<programlisting> +EXEC SQL AT <replaceable>connection-name</replaceable> DECLARE <replaceable>statement-name</replaceable> STATEMENT; +EXEC SQL PREPARE <replaceable>statement-name</replaceable> FROM :<replaceable>dyn-string</replaceable>; +</programlisting> + Once you link a sql identifier to a connection, you execute a dynamic SQL + without AT clause. + </para> The above code part should be moved to the below of the following code location and provide some example usage of it in the example code will be better. <programlisting> EXEC SQL SET CONNECTION <replaceable>connection-name</replaceable>; </programlisting> + <para> + <command>DELARE CURSOR</command> with a SQL statement identifier can be written before PREPARE. + </para> what is the need of providing details about DECLARE CURSOR here? + <para> + AT clause cannot be used with the SQL statement which have been identified by <command>DECLARE STATEMENT</command> + </para> I didn't clearly understand the limitation here, If you can provide an example here, it will be good. The test patch looks good to me. Regards, Hari Babu Fujitsu Australia