Found new issues with latest patch:

> Thank you for reviewing the patch with variable cases.
> I have revised the patch, and attached latest patch.
> > A:
> > Will you please explain the idea behind these changes ?
> I thought wrong about adding new to tail of query_buf.
> The latest patch does not change related to them.
> Thanks.

> > B:
> I added the condition of cur_line < 0.

However, this introduced new bug. As I told, when editor number of lines
reaches INT_MAX it starts giving negative number. You tried overcoming this
issue by adding "< 0" check. But I guess you again fumbled in setting that
correctly. You are setting it to INT_MAX - 1. This enforces each new line
to show line number as INT_MAX - 1 which is incorrect.

postgres=# \set PROMPT1 '%/[%l]%R%# '
postgres[1]=# \set PROMPT2 '%/[%l]%R%# '
postgres[1]=# \e
postgres[2147483646]-# limit
postgres[2147483646]-# 1;
(1 row)

postgres[1]=# \e
postgres[2147483646]-# =
postgres[2147483646]-# '
postgres[2147483646]'# abc
postgres[2147483646]'# '
postgres[2147483646]-# ;
(0 rows)

postgres[1]=# select

Again to mimic that, I have simply intialized newline to INT_MAX - 2.
Please don't take me wrong, but it seems that your unit testing is not
enough. Above issue I discovered by doing exactly same steps I did in
reviewing previous patch. If you had tested your new patch with those steps
I guess you have caught that yourself.


+             /* Calculate the line number */
+             if (scan_result != PSCAN_INCOMPLETE)
+             {
+                 /* The one new line is always added to tail of query_buf
+                 newline = (newline != 0) ? (newline + 1) : 1;
+                 cur_line += newline;
+             }

Here in above code changes, in any case you are adding 1 to newline. i.e.
when it is 0 you are setting it 1 (+1) and when > 0 you are setting nl + 1
(again +1).
So why can't you simply use"
    if (scan_result != PSCAN_INCOMPLETE)
        cur_line += (newline + 1);

Or better, why can't you initialize newline with 1 itself and then directly
assign cur_line with newline. That will eliminate above entire code block,
isn't it?
Or much better, simply get rid of newline, and use cur_line itself, will
this work well for you?

C. Typos:
/* Count the number of new line for calculate ofline number */

Missing space between 'of' and 'line'.
However better improve that to something like (just suggestion):
"Count the number of new lines to correctly adjust current line number"

/* Avoid cur_line and newline exceeds the INT_MAX */

You are saying avoid cur_line AND newline, but there is no adjustment for
newline in the code following the comment.

Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Reply via email to