Обсуждение: A few pgindent oddities
Since we were breaking our usual rule by re-indenting the 8.1 branch,
I took the time to eyeball the whole "cvs diff" for changes that weren't
just comment block fixes. I found a few things that need attention.
This change is disturbing first because it seems completely unnecessary,
and second because I'm not convinced that every C preprocessor will deal
correctly with a comment continued off a #endif line:
Index: contrib/pgbench/pgbench.c
***************
*** 1110,1116 **** fprintf(stderr, "Use limit/ulimt to increase the limit before using
pgbench.\n"); exit(1); }
! #endif /* #if !(defined(__CYGWIN__) || defined(__MINGW32__)) */ break; case 'C':
is_connect = 1;
--- 1110,1117 ---- fprintf(stderr, "Use limit/ulimt to increase the limit before using
pgbench.\n"); exit(1); }
! #endif /* #if !(defined(__CYGWIN__) ||
! * defined(__MINGW32__)) */ break; case 'C':
is_connect = 1;
This change seems odd and unnecessary as well:
Index: src/backend/access/transam/slru.c
***************
*** 252,258 **** /* indeed, the I/O must have failed */ if (shared->page_status[slotno] ==
SLRU_PAGE_READ_IN_PROGRESS) shared->page_status[slotno] = SLRU_PAGE_EMPTY;
! else /* write_in_progress */ { shared->page_status[slotno] =
SLRU_PAGE_VALID; shared->page_dirty[slotno] = true;
--- 253,260 ---- /* indeed, the I/O must have failed */ if (shared->page_status[slotno] ==
SLRU_PAGE_READ_IN_PROGRESS) shared->page_status[slotno] = SLRU_PAGE_EMPTY;
! else
! /* write_in_progress */ { shared->page_status[slotno] = SLRU_PAGE_VALID;
shared->page_dirty[slotno] = true;
And what happened here?
Index: src/interfaces/libpq/libpq-fe.h
***************
*** 35,41 **** /* Application-visible enum types */
! typedef enum { /* * Although it is okay to add to this list, values which become unused
--- 35,41 ---- /* Application-visible enum types */
! typedef enum { /* * Although it is okay to add to this list, values which become unused
regards, tom lane
Tom Lane wrote:
> Since we were breaking our usual rule by re-indenting the 8.1 branch,
> I took the time to eyeball the whole "cvs diff" for changes that weren't
> just comment block fixes. I found a few things that need attention.
>
> This change is disturbing first because it seems completely unnecessary,
> and second because I'm not convinced that every C preprocessor will deal
> correctly with a comment continued off a #endif line:
>
I don't understand why this first problem happened. Alvaro and I talked
about it but I could not determine the cause. I did not want to modify
the indent code at this stage just to fix it. I will look for a fix
later.
> Index: contrib/pgbench/pgbench.c
> ***************
> *** 1110,1116 ****
> fprintf(stderr, "Use limit/ulimt to increase the limit before using pgbench.\n");
> exit(1);
> }
> ! #endif /* #if !(defined(__CYGWIN__) || defined(__MINGW32__)) */
> break;
> case 'C':
> is_connect = 1;
> --- 1110,1117 ----
> fprintf(stderr, "Use limit/ulimt to increase the limit before using pgbench.\n");
> exit(1);
> }
> ! #endif /* #if !(defined(__CYGWIN__) ||
> ! * defined(__MINGW32__)) */
> break;
> case 'C':
> is_connect = 1;
>
>
> This change seems odd and unnecessary as well:
This is caused by this part of pgindent:
# workaround for indent bug with 'else' handling# indent comment so BSD indent will move it sed 's;\([}
]\)else[ ]*\(/\*.*\)$;\1else\ \2;g' |
The problem is that BSD indent crashes on:
else /* test */{
Not sure why, but I guess I can fix it some day. :-)
> Index: src/backend/access/transam/slru.c
> ***************
> *** 252,258 ****
> /* indeed, the I/O must have failed */
> if (shared->page_status[slotno] == SLRU_PAGE_READ_IN_PROGRESS)
> shared->page_status[slotno] = SLRU_PAGE_EMPTY;
> ! else /* write_in_progress */
> {
> shared->page_status[slotno] = SLRU_PAGE_VALID;
> shared->page_dirty[slotno] = true;
> --- 253,260 ----
> /* indeed, the I/O must have failed */
> if (shared->page_status[slotno] == SLRU_PAGE_READ_IN_PROGRESS)
> shared->page_status[slotno] = SLRU_PAGE_EMPTY;
> ! else
> ! /* write_in_progress */
> {
> shared->page_status[slotno] = SLRU_PAGE_VALID;
> shared->page_dirty[slotno] = true;
>
>
> And what happened here?
I saw this one to and was stumped at the cause. We have other 'typedef
enum' lines in the code which were not mangled, just this one. Again,
needs research.
> Index: src/interfaces/libpq/libpq-fe.h
> ***************
> *** 35,41 ****
>
> /* Application-visible enum types */
>
> ! typedef enum
> {
> /*
> * Although it is okay to add to this list, values which become unused
> --- 35,41 ----
>
> /* Application-visible enum types */
>
> ! typedef enum
> {
> /*
> * Although it is okay to add to this list, values which become unused
>
>
> regards, tom lane
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
Bruce Momjian wrote:
> > And what happened here?
>
> I saw this one to and was stumped at the cause. We have other 'typedef
> enum' lines in the code which were not mangled, just this one. Again,
> needs research.
>
> > Index: src/interfaces/libpq/libpq-fe.h
> > ***************
> > *** 35,41 ****
> >
> > /* Application-visible enum types */
> >
> > ! typedef enum
> > {
> > /*
> > * Although it is okay to add to this list, values which become unused
> > --- 35,41 ----
> >
> > /* Application-visible enum types */
> >
> > ! typedef enum
> > {
> > /*
> > * Although it is okay to add to this list, values which become unused
Ah the cause of this one is this at the top of src/interfaces/libpq/libpq-fe.h:
#ifdef __cplusplusextern "C"{#endif
Not sure I can blame pgindent. Of course the fact that the other
'typedef enum' lines in the file are not indented isn't consistent.
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073
Bruce Momjian wrote:
> Tom Lane wrote:
> > Since we were breaking our usual rule by re-indenting the 8.1 branch,
> > I took the time to eyeball the whole "cvs diff" for changes that weren't
> > just comment block fixes. I found a few things that need attention.
> >
> > This change is disturbing first because it seems completely unnecessary,
> > and second because I'm not convinced that every C preprocessor will deal
> > correctly with a comment continued off a #endif line:
> >
>
> I don't understand why this first problem happened. Alvaro and I talked
> about it but I could not determine the cause. I did not want to modify
> the indent code at this stage just to fix it. I will look for a fix
> later.
I removed the comment. Let's see if we hit it again.
> > Index: contrib/pgbench/pgbench.c
> > ***************
> > *** 1110,1116 ****
> > fprintf(stderr, "Use limit/ulimt to increase the limit before using pgbench.\n");
> > exit(1);
> > }
> > ! #endif /* #if !(defined(__CYGWIN__) || defined(__MINGW32__)) */
> > break;
> > case 'C':
> > is_connect = 1;
> > --- 1110,1117 ----
> > fprintf(stderr, "Use limit/ulimt to increase the limit before using pgbench.\n");
> > exit(1);
> > }
> > ! #endif /* #if !(defined(__CYGWIN__) ||
> > ! * defined(__MINGW32__)) */
> > break;
> > case 'C':
> > is_connect = 1;
> >
> >
> > This change seems odd and unnecessary as well:
>
> I saw this one to and was stumped at the cause. We have other 'typedef
> enum' lines in the code which were not mangled, just this one. Again,
> needs research.
I fixed this one by hacking pgindent script to left-justify all typedefs
in that file only.
> > Index: src/interfaces/libpq/libpq-fe.h
> > ***************
> > *** 35,41 ****
> >
> > /* Application-visible enum types */
> >
> > ! typedef enum
> > {
> > /*
> > * Although it is okay to add to this list, values which become unused
> > --- 35,41 ----
> >
> > /* Application-visible enum types */
> >
> > ! typedef enum
> > {
> > /*
> > * Although it is okay to add to this list, values which become unused
> >
> >
> > regards, tom lane
> >
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman@candle.pha.pa.us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square,
Pennsylvania19073