Thread: 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