On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel.steh...@gmail.com> wrote: > I hope so I found and fixed last issue - the longer functions was > showed directly - without a pager.
As a matter of style, I suggest leaving bool *edited as the last argument to do_edit() and inserting int lineno as the second-to-last argument. ! int lineno = -1; [...] ! if (atoi(ln) < 1) ! { ! psql_error("invalid line number\n"); ! status = PSQL_CMD_ERROR; ! } ! else ! lineno = atoi(ln); Why call atoi(n) twice? Can't you just write: lineno = atoi(n); if (lineno < 1) { ...error stuff... } I suggested writing psql_assert(datag != NULL) rather than just psql_assert(datag). Instead of: cannot use a editor navigation without setting EDITOR_LINENUMBER_SWITCH variable I suggest: cannot edit a specific line because the EDITOR_LINENUMBER_SWITCH variable is not set I don't find the name get_dg_tag() to be very mnemonic. How about get_function_dollarquote_tag()? In help.c, it looks like you've only added one line but incremented the pager count by three. In this bit: ! dqtag = get_dq_tag(lines); ! if (dqtag) ! { ! free(dqtag); ! break; ! } ! else ! lineno++; The "else" is unnecessary. And just after that: ! if (end_of_line) ! lines = end_of_line + 1; ! else ! break; You can write this more cleanly by saying if (!end_of_line) break; lines = end_of_line + 1. The following diff hunk (2213,2218) just adds a blank line and is therefore unnecessary. There's a similar hunk in the docs portion of the patch. In this part: func = psql_scan_slash_option(scan_state, OT_WHOLE_LINE, NULL, true); ! lineno = extract_lineno_from_funcdesc(func, &status); ! ! if (status != PSQL_CMD_ERROR) { ! if (!func) ! { ! /* set up an empty command to fill in */ ! printfPQExpBuffer(query_buf, ! "CREATE FUNCTION ( )\n" ! " RETURNS \n" ! " LANGUAGE \n" ! " -- common options: IMMUTABLE STABLE STRICT SECURITY DEFINER\n" ! "AS $function$\n" ! "\n$function$\n"); ! } ! else if (!lookup_function_oid(pset.db, func, &foid)) ! { ! /* error already reported */ ! status = PSQL_CMD_ERROR; ! } ! else if (!get_create_function_cmd(pset.db, foid, query_buf)) ! { ! /* error already reported */ ! status = PSQL_CMD_ERROR; ! } ! if (func) ! free(func); } Why is it correct for if (func) free(func) to be inside the if (status != PSQL_CMD_ERROR) block? It looks to me like func gets initialized first, before status is set. It seems unnecessary for extract_lineno_from_funcdesc() to return the line number and have a separate out parameter to return a backslashResult. Can't you just return -1 to indicate an error? (You might need to move the if (!func) test at the top to the caller, but that seems OK.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers