From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Jan Urbański <wulczer(at)wulczer(dot)org>, Postgres - Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: review: psql: edit function, show function commands patch |
Date: | 2010-08-04 12:53:15 |
Message-ID: | AANLkTinpjRjgrXgOc1FC1tPGAcG_axVB81FPYNk4C67-@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Aug 3, 2010 at 7:20 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Dunstan | 2010-08-04 12:56:17 | Re: tracking inherited columns (was: patch for check constraints using multiple inheritance) |
Previous Message | Heikki Linnakangas | 2010-08-04 12:23:36 | Re: merge command - GSoC progress |