Thread: Minor bugs and a formatting gripe

Minor bugs and a formatting gripe

From
Tom Lane
Date:
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);

Re: [HACKERS] Minor bugs and a formatting gripe

From
Bruce Momjian
Date:
> 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)

Re: [HACKERS] Minor bugs and a formatting gripe

From
Bruce Momjian
Date:
> 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)

Re: [HACKERS] Minor bugs and a formatting gripe

From
Tom Lane
Date:
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

Re: [HACKERS] Minor bugs and a formatting gripe

From
Bruce Momjian
Date:
> 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)