More pgindent follies - Mailing list pgsql-hackers

As long as you're fixing bugs in pgindent, here are some more follies
exhibited by the most recent pgindent run.  Some of these bugs have
been there for awhile, but at least one (the removal of space before
a same-line comment) seems to be new as of the most recent run.

The examples are all taken from 

$ cd .../src/backend/utils/adt/
$ cvs diff -c -r1.85 -r1.86 selfuncs.c

but there are many similar cases elsewhere.

1. I can sort of understand inserting space before an #ifdef (though I do
not agree with it), but why before #endif?  Why does the first example
insert a blank line *only* before the #endif and not its matching #if?
If you're trying to set off the #if block from surrounding code, seems
like a blank line *after* the #endif would help, not one before.  But
none of the here-inserted blank lines seem to me to improve readability;
I'd vote for not making any such changes.

***************
*** 648,653 ****
--- 653,659 ---- { #ifdef NOT_USED                    /* see neqjoinsel() before removing me! */     Oid
opid= PG_GETARG_OID(0);
 
+  #endif     Oid            relid1 = PG_GETARG_OID(1);     AttrNumber    attno1 = PG_GETARG_INT16(2);
***************
*** 1078,1087 ****
--- 1091,1102 ---- convert_string_datum(Datum value, Oid typid) {     char       *val;
+  #ifdef USE_LOCALE     char       *xfrmstr;     size_t        xfrmsize;     size_t        xfrmlen;
+  #endif      switch (typid)
***************

2. Here are two examples of a long-standing bug: pgindent frequently
(but not always) mis-indents the first 'case' line(s) of a switch.
I don't agree with its indentation of block comments between case
entries, either.

***************
*** 845,892 ****     switch (valuetypid)     { 
!         /*
!          * Built-in numeric types
!          */
!         case BOOLOID:
!         case INT2OID:
!         case INT4OID:
!         case INT8OID:
!         case FLOAT4OID:
!         case FLOAT8OID:
!         case NUMERICOID:
!         case OIDOID:
!         case REGPROCOID:             *scaledvalue = convert_numeric_to_scalar(value, valuetypid);
*scaledlobound= convert_numeric_to_scalar(lobound, boundstypid);             *scaledhibound =
convert_numeric_to_scalar(hibound,boundstypid);             return true; 
 
!         /*
!          * Built-in string types
!          */         case CHAROID:         case BPCHAROID:         case VARCHAROID:
--- 851,898 ----     switch (valuetypid)     { 
!             /*
!              * Built-in numeric types
!              */
!             case BOOLOID:
!             case INT2OID:
!             case INT4OID:
!             case INT8OID:
!             case FLOAT4OID:
!             case FLOAT8OID:
!             case NUMERICOID:
!             case OIDOID:
!             case REGPROCOID:             *scaledvalue = convert_numeric_to_scalar(value, valuetypid);
*scaledlobound= convert_numeric_to_scalar(lobound, boundstypid);             *scaledhibound =
convert_numeric_to_scalar(hibound,boundstypid);             return true; 
 
!             /*
!              * Built-in string types
!              */         case CHAROID:         case BPCHAROID:         case VARCHAROID:
***************
*** 911,917 **** {     switch (typid)     {
!         case BOOLOID:             return (double) DatumGetBool(value);         case INT2OID:             return
(double)DatumGetInt16(value);
 
--- 917,923 ---- {     switch (typid)     {
!             case BOOLOID:             return (double) DatumGetBool(value);         case INT2OID:             return
(double)DatumGetInt16(value);
 
***************

3. This is new misbehavior as of the last pgindent run: whitespace
between a statement and a comment on the same line is sometimes removed.

***************
*** 1120,1126 ****  #ifdef USE_LOCALE     /* Guess that transformed string is not much bigger than original */
!     xfrmsize = strlen(val) + 32;        /* arbitrary pad value here... */     xfrmstr = (char *) palloc(xfrmsize);
xfrmlen = strxfrm(xfrmstr, val, xfrmsize);     if (xfrmlen >= xfrmsize)
 
--- 1137,1143 ----  #ifdef USE_LOCALE     /* Guess that transformed string is not much bigger than original */
!     xfrmsize = strlen(val) + 32;/* arbitrary pad value here... */     xfrmstr = (char *) palloc(xfrmsize);
xfrmlen= strxfrm(xfrmstr, val, xfrmsize);     if (xfrmlen >= xfrmsize)
 
***************

4. This breaking of a comment attached to a #define scares me.
Apparently gcc still thinks the comment is valid, but it seems to me
that this is making not-very-portable assumptions about the behavior of
the preprocessor.  I always thought that you needed to use backslashes
to continue a preprocessor command onto the next line reliably.

***************
*** 1691,1705 ****  #define FIXED_CHAR_SEL    0.04    /* about 1/25 */ #define CHAR_RANGE_SEL    0.25
! #define ANY_CHAR_SEL    0.9        /* not 1, since it won't match end-of-string */ #define FULL_WILDCARD_SEL 5.0
#definePARTIAL_WILDCARD_SEL 2.0 
 
--- 1718,1733 ----  #define FIXED_CHAR_SEL    0.04    /* about 1/25 */ #define CHAR_RANGE_SEL    0.25
! #define ANY_CHAR_SEL    0.9        /* not 1, since it won't match
!                                  * end-of-string */ #define FULL_WILDCARD_SEL 5.0 #define PARTIAL_WILDCARD_SEL 2.0 
***************

5. Here's an interesting one: why was the "return" line misindented?
I don't particularly agree with the handling of the comments on the
#else and #endif lines either... they're not even mutually consistent,
let alone stylistically good.

***************
*** 1904,1912 ****     else         result = false;     return (bool) result;
! #else /* not USE_LOCALE */
!     return true;                /* We must be in C locale, which is OK */
! #endif /* USE_LOCALE */ }  /*
--- 1935,1943 ----     else         result = false;     return (bool) result;
! #else                            /* not USE_LOCALE */
!                 return true;    /* We must be in C locale, which is OK */
! #endif     /* USE_LOCALE */ }  /*
***************
        regards, tom lane