Thread: Minor bugs and a formatting gripe
Bruce mentioned that he'd had some trouble applying my last set of
patches, because he'd already run his reformatting script against
the files involved. So, I took the trouble to grovel through a
"diff --ignore-all-space" comparison of tonight's cvs tree with what
I had last week. (I didn't have the energy to go through the *whole*
tree, but I did look through all of src/interfaces/.) It was a
worthwhile exercise because I found a couple of minor errors --- see
attached patch.
I was mostly pretty happy with what the formatting script had done with
the code, especially in libpgtcl which had not been anywhere close to
matching the formatting conventions of the rest of pgsql. Nice job!
BUT: I am not happy that the script chooses to reflow block comments.
Line breaks chosen on the basis of how much fits are not a readability
improvement over line breaks placed where a human author felt they made
sense. For example, is this really an improvement in readability?
(And if it's *not* a clear gain, why are we doing it?)
*** 201,209 ****
/* === in fe-misc.c === */
! /* "Get" and "Put" routines return 0 if successful, EOF if not.
! * Note that for Get, EOF merely means the buffer is exhausted,
! * not that there is necessarily any error.
*/
extern int pqGetc(char *result, PGconn *conn);
extern int pqGets(char *s, int maxlen, PGconn *conn);
--- 201,210 ----
/* === in fe-misc.c === */
! /*
! * "Get" and "Put" routines return 0 if successful, EOF if not. Note that
! * for Get, EOF merely means the buffer is exhausted, not that there is
! * necessarily any error.
*/
extern int pqGetc(char *result, PGconn *conn);
extern int pqGets(char *s, int maxlen, PGconn *conn);
***************
The only case within src/interfaces for which I am sufficiently annoyed
to submit a reversing patch is this one:
*** 1462,1476 ****
if (strncmp(res->cmdStatus, "INSERT ", 7) != 0)
return "";
! /* The cmdStatus string looks like
! * INSERT oid count\0
! * In order to be able to return an ordinary C string without
! * damaging the result for PQcmdStatus or PQcmdTuples, we copy
! * the oid part of the string to just after the null, so that
! * cmdStatus looks like
! * INSERT oid count\0oid\0
! * ^ our return value points here
! * Pretty klugy eh? This routine should've just returned an Oid value.
*/
slen = strlen(res->cmdStatus);
--- 1488,1500 ----
if (strncmp(res->cmdStatus, "INSERT ", 7) != 0)
return "";
! /*
! * The cmdStatus string looks like INSERT oid count\0 In order to be
! * able to return an ordinary C string without damaging the result for
! * PQcmdStatus or PQcmdTuples, we copy the oid part of the string to
! * just after the null, so that cmdStatus looks like INSERT oid
! * count\0oid\0 ^ our return value points here Pretty klugy eh? This
! * routine should've just returned an Oid value.
*/
slen = strlen(res->cmdStatus);
***************
but I hope this one drives home the point that automatic reflow of
block comments is a pretty stupid idea. I strongly recommend that
that "feature" be disabled forthwith. I'd further recommend that
someone look through the full source tree to see if any other block
comments have been rendered equally illegible. I haven't got time
to do it myself right now, however.
Patch attached that corrects two minor errors introduced into
libpgtcl and libpq, and corrects the above loss of readability
of one block comment.
regards, tom lane
*** src/interfaces/libpgtcl/pgtclCmds.c.orig Thu Sep 3 01:08:28 1998
--- src/interfaces/libpgtcl/pgtclCmds.c Thu Sep 3 23:06:36 1998
***************
*** 616,623 ****
for (i = 1; i < PQnfields(result); i++)
{
sprintf(workspace, "%s,%.200s%s", field0, PQfname(result,i),
! appendstr);
! sprintf(workspace, "%s,%.200s", field0, PQfname(result,i));
if (Tcl_SetVar2(interp, arrVar, workspace,
PQgetvalue(result, tupno, i),
TCL_LEAVE_ERR_MSG) == NULL)
--- 616,622 ----
for (i = 1; i < PQnfields(result); i++)
{
sprintf(workspace, "%s,%.200s%s", field0, PQfname(result,i),
! appendstr);
if (Tcl_SetVar2(interp, arrVar, workspace,
PQgetvalue(result, tupno, i),
TCL_LEAVE_ERR_MSG) == NULL)
*** src/interfaces/libpq/libpq-fe.h.orig Wed Sep 2 22:10:51 1998
--- src/interfaces/libpq/libpq-fe.h Thu Sep 3 23:37:43 1998
***************
*** 182,188 ****
extern ConnStatusType PQstatus(PGconn *conn);
extern char *PQerrorMessage(PGconn *conn);
extern int PQsocket(PGconn *conn);
- extern int PQsocket(PGconn *conn);
extern int PQbackendPID(PGconn *conn);
/* Enable/disable tracing */
--- 182,187 ----
*** src/interfaces/libpq/fe-exec.c.orig Wed Sep 2 22:10:47 1998
--- src/interfaces/libpq/fe-exec.c Thu Sep 3 23:33:42 1998
***************
*** 1489,1500 ****
return "";
/*
! * The cmdStatus string looks like INSERT oid count\0 In order to be
! * able to return an ordinary C string without damaging the result for
! * PQcmdStatus or PQcmdTuples, we copy the oid part of the string to
! * just after the null, so that cmdStatus looks like INSERT oid
! * count\0oid\0 ^ our return value points here Pretty klugy eh? This
! * routine should've just returned an Oid value.
*/
slen = strlen(res->cmdStatus);
--- 1489,1503 ----
return "";
/*
! * The cmdStatus string looks like
! * INSERT oid count\0
! * In order to be able to return an ordinary C string without
! * damaging the result for PQcmdStatus or PQcmdTuples, we copy
! * the oid part of the string to just after the null, so that
! * cmdStatus looks like
! * INSERT oid count\0oid\0
! * ^ our return value points here
! * Pretty klugy eh? This routine should've just returned an Oid value.
*/
slen = strlen(res->cmdStatus);
> Bruce mentioned that he'd had some trouble applying my last set of
> patches, because he'd already run his reformatting script against
> the files involved. So, I took the trouble to grovel through a
> "diff --ignore-all-space" comparison of tonight's cvs tree with what
> I had last week. (I didn't have the energy to go through the *whole*
> tree, but I did look through all of src/interfaces/.) It was a
> worthwhile exercise because I found a couple of minor errors --- see
> attached patch.
>
> I was mostly pretty happy with what the formatting script had done with
> the code, especially in libpgtcl which had not been anywhere close to
> matching the formatting conventions of the rest of pgsql. Nice job!
>
> BUT: I am not happy that the script chooses to reflow block comments.
> Line breaks chosen on the basis of how much fits are not a readability
> improvement over line breaks placed where a human author felt they made
> sense. For example, is this really an improvement in readability?
> (And if it's *not* a clear gain, why are we doing it?)
Good point. The way to control this is to use:
/*--------------
* bla bla
*--------------
*/
This will not be reformatted by the system. When I first ran pgindent,
I checked all the diffs (whew), and found many box comments that had
that format that were not affected. Other comments got gobbled, and I
backed out the change and added ----- to the comments. If you see the
indent manual page, you will see a mention of this.
`Box' comments. Indent assumes that any comment with a dash or star imme-
diately after the start of comment (that is, `/*-' or `/**') is a comment
surrounded by a box of stars. Each line of such a comment is left un-
changed, except that its indentation may be adjusted to account for the
change in indentation of the first line of the comment.
So this is the way to preserve comment line numbering, and you will see
the main code does handle this properly, and that other code that does
not have the box gets reformatted as the code changes to look pretty.
Do you still want me to turn it off?
--
Bruce Momjian | 830 Blythe Avenue
maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)
> Bruce mentioned that he'd had some trouble applying my last set of > patches, because he'd already run his reformatting script against > the files involved. So, I took the trouble to grovel through a > "diff --ignore-all-space" comparison of tonight's cvs tree with what > I had last week. (I didn't have the energy to go through the *whole* > tree, but I did look through all of src/interfaces/.) It was a > worthwhile exercise because I found a couple of minor errors --- see > attached patch. > > I was mostly pretty happy with what the formatting script had done with > the code, especially in libpgtcl which had not been anywhere close to > matching the formatting conventions of the rest of pgsql. Nice job! > > BUT: I am not happy that the script chooses to reflow block comments. > Line breaks chosen on the basis of how much fits are not a readability > improvement over line breaks placed where a human author felt they made > sense. For example, is this really an improvement in readability? > (And if it's *not* a clear gain, why are we doing it?) Thanks for checking on my patch job. Patches applied. -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)
Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> BUT: I am not happy that the script chooses to reflow block comments.
> `Box' comments. Indent assumes that any comment with a dash or
> star imme- diately after the start of comment (that is, `/*-' or
> `/**') is a comment surrounded by a box of stars. Each line of
> such a comment is left un- changed, except that its indentation
> may be adjusted to account for the change in indentation of the
> first line of the comment.
> Do you still want me to turn it off?
Ah. OK, learn something new every day ;-). Now that I know how to
control it, I can live with it.
A suggestion: it'd be worth making an entry in FAQ_DEV that talks about
coding style conventions and points out that people should expect to
have their code reformatted by pgindent. As I don't have indent
installed on my system, I have very little idea what to expect from it;
I can't readily consult the man page.
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes: > >> BUT: I am not happy that the script chooses to reflow block comments. > > > `Box' comments. Indent assumes that any comment with a dash or > > star imme- diately after the start of comment (that is, `/*-' or > > `/**') is a comment surrounded by a box of stars. Each line of > > such a comment is left un- changed, except that its indentation > > may be adjusted to account for the change in indentation of the > > first line of the comment. > > > Do you still want me to turn it off? > > Ah. OK, learn something new every day ;-). Now that I know how to > control it, I can live with it. > > A suggestion: it'd be worth making an entry in FAQ_DEV that talks about > coding style conventions and points out that people should expect to > have their code reformatted by pgindent. As I don't have indent > installed on my system, I have very little idea what to expect from it; > I can't readily consult the man page. OK, new pgindent entry is: pgindent is run on all source files just before each beta test period. It auto-formats all source files to make them consistent. Comment blocks that need specific line breaks should be formatted as block comments, where the comment starts as /*------. These comments will not be reformatted in any way. -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)