Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.) - Mailing list pgsql-hackers

I wrote:
> Curiously, there are other enum declarations that don't get the phony
> extra indentation.  I traced through it a bit and eventually found that
> the difference between OK and not OK is that the declarations that don't
> get messed up look like "typedef enum enumlabel ...", ie the problem
> with this one is there's no extra identifier after "enum".  The proximate
> cause of that is this line in indent.c:

>       ps.in_decl = ps.decl_on_line = ps.last_token != type_def;

I found that things seem to work better with this change:

@@ -939,7 +938,7 @@ check_type:       }       ps.in_or_st = true; /* this might be a structure or initialization
       * declaration */ 
-       ps.in_decl = ps.decl_on_line = ps.last_token != type_def;
+       ps.in_decl = ps.decl_on_line = (ps.last_token != type_def || type_code == structure);       if ( /*
!ps.in_or_st&& */ ps.dec_nest <= 0)       ps.just_saw_decl = 2;       prefix_blankline_requested = 0; 

I have no idea if that's a correct or sufficient fix, but it makes the
oddnesses around unnamed enum and struct declarations in the PG sources
go away.

There remain a small number of cases that look like bugs.  One is the
case I showed before:

@@ -531,7 +531,7 @@ static boolean2string(ean13 ean, bool errorOK, char *result, bool shortType){   const char
*(*TABLE)[2];
-   const unsigned (*TABLE_index)[2];
+   const unsigned(*TABLE_index)[2];   enum isn_type type = INVALID;   char       *aux;

I have an impression that this might be related to the ps.in_decl business,
but the patch shown above did not change it.  Possibly related is this:

diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 986fe6e..a2d11e5 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -149,8 +149,8 @@ typedef struct GinBtreeData   bool        (*findItem) (GinBtree, GinBtreeStack *);   /* insert
methods*/ 
-   OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
-   GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page
*);
+   OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
+   GinPlaceToPageRC(*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page
*);  void        (*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void *);   void
*(*prepareDownlink)(GinBtree, Buffer);   void        (*fillRoot) (GinBtree, Page, BlockNumber, Page, BlockNumber,
Page);

Whatever's going on there, it seems to affect only a very small number
of places.  No idea why.

Also, multiple comments on the same line are now run together, which
is surely not better:

@@ -2144,11 +2144,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)        */       NewPage->xlp_magic =
XLOG_PAGE_MAGIC;
-       /* NewPage->xlp_info = 0; */    /* done by memset */
+       /* NewPage->xlp_info = 0; *//* done by memset */       NewPage->xlp_tli = ThisTimeLineID;
NewPage->xlp_pageaddr= NewPageBeginPtr; 
-       /* NewPage->xlp_rem_len = 0; */ /* done by memset */
+       /* NewPage->xlp_rem_len = 0; *//* done by memset */       /*        * If online backup is not in progress, mark
theheader to indicate 

Also, I found two places where an overlength comment line is simply busted
altogether --- notice that a character is missing at the split point:

@@ -257,7 +257,8 @@ get_pgpid(bool is_status_request)       /*        * The Linux Standard Base Core Specification 3.1
saysthis should        * return '4, program or service status is unknown' 
-        * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
+        * https://refspecs.linu
+        * base.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g        * eneric/iniscrptact.html        */
exit(is_status_request? 4 : 1); 

@@ -629,7 +629,8 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)                   /*
               * This code erroneously assumes '\.' on a line alone                    * inside a quoted CSV string
terminatesthe \copy. 
-                    * http://www.postgresql.org/message-id/E1TdNVQ-0001ju-GO@w
+                    * http://w
+                    * w.postgresql.org/message-id/E1TdNVQ-0001ju-GO@w                    * rigleys.postgresql.org
             */                   if (strcmp(buf, "\\.\n") == 0 || 

Another problem is that the handling of unrecognized typedef names as
field types in a struct has gotten significantly worse:

diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index a35addf..919d262 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -114,14 +114,14 @@ typedef struct SH_TYPE   uint32      grow_threshold;   /* hash buckets */
-   SH_ELEMENT_TYPE *data;
+               SH_ELEMENT_TYPE * data;   /* memory context to use for allocations */   MemoryContext ctx;

I don't mind the space after the star so much, but the indentation
is awful.  In this particular example we *cannot* fix it by making
SH_ELEMENT_TYPE a real typedef, because it's actually meant to be
a macro and vary from one inclusion of simplehash.h to another.

I guess a possible answer is to create a list of "manual" typedef
names to add to the ones extracted from the buildfarm.  But if
it's possible for indent to be a bit more fail-soft in this type
of case, we'd not have to.  It seems like other users of indent
might have more need for plausible treatment of unknown typedef
names than we do, too.
        regards, tom lane