Re: review: psql: edit function, show function commands patch - Mailing list pgsql-hackers
From | Pavel Stehule |
---|---|
Subject | Re: review: psql: edit function, show function commands patch |
Date | |
Msg-id | AANLkTimo9krX2s-Fv4iVs-342rZYCtH_so2=Sn4M4yae@mail.gmail.com Whole thread Raw |
In response to | Re: review: psql: edit function, show function commands patch (Robert Haas <robertmhaas@gmail.com>) |
Responses |
Re: review: psql: edit function, show function commands patch
|
List | pgsql-hackers |
Hello updated patch attached 2010/8/4 Robert Haas <robertmhaas@gmail.com>: > On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel.stehule@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... > } fixed > > 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 fixed > > I don't find the name get_dg_tag() to be very mnemonic. How about > get_function_dollarquote_tag()? > I used get_functiondef_dollarquote_tag > In help.c, it looks like you've only added one line but incremented > the pager count by three. > The original value for pager is probably wrong (not actual). I rechecked real row numbers in external editor. > 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. > fixed > 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. fixed > > 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. fixed > > 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.) I can't to do it. There are three states 1) lineno is wrong 2) lineno is undefined 3) lineno is defined @1 is solved via backslashResult, @2 lineno is negative, @3 lineno is positive Regards Pavel > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise Postgres Company >
Attachment
pgsql-hackers by date: