From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(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 14:35:25 |
Message-ID: | AANLkTimo9krX2s-Fv4iVs-342rZYCtH_so2=Sn4M4yae@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello
updated patch attached
2010/8/4 Robert Haas <robertmhaas(at)gmail(dot)com>:
> 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...
> }
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 | Content-Type | Size |
---|---|---|
edit6.diff | text/x-patch | 21.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2010-08-04 14:35:55 | Re: review: psql: edit function, show function commands patch |
Previous Message | Thom Brown | 2010-08-04 14:31:09 | Re: string_agg delimiter having no effect with order by |