Обсуждение: Re: [PATCH] pg_bsd_indent: improve formatting of multiline comments

On Fri, Jun 20, 2025 at 04:44:08PM +0300, Aleksander Alekseev wrote:
> Hi Arseniy,
> 
> > I tried it with the whole project and almost always it works great.
> > But I noticed two cases where it works probably not as expected:
> >
> > 1) comments which don't have a star on each line. For example:
> > file 'cube.c'
> >
> > before:
> > /* make up a metric in which one box will be 'lower' than the other
> >    -- this can be useful for sorting and to determine uniqueness */
> >
> > after:
> > /*
> >  * make up a metric in which one box will be 'lower' than the other
> >    -- this can be useful for sorting and to determine uniqueness */
> >
> > 2) comments where closing */ is on the last comment line. For example:
> > file 'crypt-blowfish.c'
> >
> > before:
> > /* This has to be bug-compatible with the original implementation, so
> >  * only encode 23 of the 24 bytes. :-) */
> >
> > after:
> > /*
> >  * This has to be bug-compatible with the original implementation, so
> >  * only encode 23 of the 24 bytes. :-) */
> 
> Thanks for the review. You are right, these comments shouldn't be affected.
> 
> It's going to be simpler to modify pgindent then. PFA the updated patch.

Given the quality of BSD indent code, I have _always_ found it easier to
modify pgindent.  ;-

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Fri, Jun 20, 2025 at 05:01:41PM +0300, Aleksander Alekseev wrote:
> Hi,
> 
> > Given the quality of BSD indent code, I have _always_ found it easier to
> > modify pgindent.  ;-
> 
> :D Initially I thought that the problem was simple enough to solve it
> in C, but this turned out not to be true.

I always found it ironic that a tool designed to make source code easier
to understand like BSD indent is so hard to understand.  Its use of
short variable names alone is confusing.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

On Fri, Jun 20, 2025 at 5:01 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> Hi,
>
> > Given the quality of BSD indent code, I have _always_ found it easier to
> > modify pgindent.  ;-
>
> :D Initially I thought that the problem was simple enough to solve it
> in C, but this turned out not to be true.
>
> > It's going to be simpler to modify pgindent then. PFA the updated patch.
>
> I noticed a mistake in v2. Here is the corrected patch. Changes
> comparing to the previous version:
>

Thanks!

Now it affects 4 times more files (380). What I noticed:
1) Most of the comments are bordered comments like this:
-/* --------------------------------------------------------------------------------
+/*
+ * --------------------------------------------------------------------------------
  * Public IO related functions operating on IO Handles
  * --------------------------------------------------------------------------------
  */

Do we want to skip such comments?
I have also seen comments with '====' border.

2) Some comments like this:

before:
/* Author: Linus Tolke
   (actually most if the code is "borrowed" from the distribution and just
   slightly modified)
 */

after:
/*
 * Author: Linus Tolke
   (actually most if the code is "borrowed" from the distribution and just
   slightly modified)
 */

I guess closing */ on the separate line is the trigger?
If I'm not wrong there are only 3 such comments, maybe it is easier to
fix them by hand?)

3) It seems all geqo related file contains such comment:
-/* contributed by:
+/*
+ * contributed by:
    =*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=
    *  Martin Utesch                             * Institute of
Automatic Control          *
    =


Best regards,
Arseniy Mukhin

On Mon, Jun 23, 2025 at 01:42:32PM +0300, Aleksander Alekseev wrote:
>> Do we want to skip such comments?
>> I have also seen comments with '====' border.
> 
> Personally I don't have a strong opinion on this. We can easily add an
> exception for "/* ---" and "/* ===" comments if somebody believes this
> is a problem. I choose not to add such an exception just yet only
> because I don't like unnecessary exceptions :)

+1 for adding an exception for "/* -----" style comments.  I tested running
pgindent after applying the patch, and the first thing I noticed was all
these (IMHO) unnecessary changes.  I don't think it helps readability, and
even if it did, it's arguably not worth the churn.

-- 
nathan

Hi Nathan,

> > Personally I don't have a strong opinion on this. We can easily add an
> > exception for "/* ---" and "/* ===" comments if somebody believes this
> > is a problem. I choose not to add such an exception just yet only
> > because I don't like unnecessary exceptions :)
>
> +1 for adding an exception for "/* -----" style comments.  I tested running
> pgindent after applying the patch, and the first thing I noticed was all
> these (IMHO) unnecessary changes.  I don't think it helps readability, and
> even if it did, it's arguably not worth the churn.

OK, here is the corrected patch v5.

-- 
Best regards,
Aleksander Alekseev

On Mon, Oct 27, 2025 at 03:13:33PM +0300, Aleksander Alekseev wrote:
> OK, here is the corrected patch v5.

Thanks.  I had to run pgindent twice before it stopped making changes, but
the results look pretty good to me.  However, I still noticed a few
oddities.

--- a/contrib/seg/seg.c
+++ b/contrib/seg/seg.c
@@ -2,9 +2,9 @@
  * contrib/seg/seg.c
  *
  ******************************************************************************
-  This file contains routines that can be bound to a Postgres backend and
-  called by the backend in the process of processing queries.  The calling
-  format for these routines is dictated by Postgres architecture.
+ *  This file contains routines that can be bound to a Postgres backend and
+ *  called by the backend in the process of processing queries.  The calling
+ *  format for these routines is dictated by Postgres architecture.
 ******************************************************************************/

This one is a little weird, but maybe it's okay...  It looks like pgindent
retains the spacing (or lack of spacing) in this situation.  It's probably
not worth the complexity to try to make it smarter here.

--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -69,7 +69,7 @@
 /*
  * DISABLE_LEADER_PARTICIPATION disables the leader's participation in
  * parallel index builds.  This may be useful as a debugging aid.
-#undef DISABLE_LEADER_PARTICIPATION
+ *#undef DISABLE_LEADER_PARTICIPATION
  */

IMHO we should either remove this line or move it out of the comment.
AFAICT we ordinarily don't #undef debugging stuff like this, presumably so
you can change it with compile flags.

In aclcheck.c and analyze.c, The **** and ==== lines are still getting
bumped down to a new line.

-.* This function just encapsulates the framing rules.
+ *.* This function just encapsulates the framing rules.

This looks like a goof in commit 25a30bb.  I'll go fix that one separately.

--- a/src/backend/storage/ipc/waiteventset.c
+++ b/src/backend/storage/ipc/waiteventset.c
@@ -2010,7 +2010,7 @@ ResOwnerReleaseWaitEventSet(Datum res)
  * NB: be sure to save and restore errno around it.  (That's standard practice
  * in most signal handlers, of course, but we used to omit it in handlers that
  * only set a flag.) XXX
-  *
+ *  *

This one looks like a goof in commit 393e0d2.  It's interesting that
pgindent chooses to add an extra * here, though.

-- 
nathan

On Mon, Oct 27, 2025 at 09:55:38AM -0500, Nathan Bossart wrote:
>  /*
>   * DISABLE_LEADER_PARTICIPATION disables the leader's participation in
>   * parallel index builds.  This may be useful as a debugging aid.
> -#undef DISABLE_LEADER_PARTICIPATION
> + *#undef DISABLE_LEADER_PARTICIPATION
>   */
>
> IMHO we should either remove this line or move it out of the comment.
> AFAICT we ordinarily don't #undef debugging stuff like this, presumably so
> you can change it with compile flags.

I would put this one on a separate line, outside the comment.  It's
minor, still we use this style in pg_config_manual.h.  See around
REALLOCATE_BITMAPSETS.
--
Michael

> On Oct 28, 2025, at 07:35, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Mon, Oct 27, 2025 at 09:55:38AM -0500, Nathan Bossart wrote:
>> /*
>>  * DISABLE_LEADER_PARTICIPATION disables the leader's participation in
>>  * parallel index builds.  This may be useful as a debugging aid.
>> -#undef DISABLE_LEADER_PARTICIPATION
>> + *#undef DISABLE_LEADER_PARTICIPATION
>>  */
>> 
>> IMHO we should either remove this line or move it out of the comment.
>> AFAICT we ordinarily don't #undef debugging stuff like this, presumably so
>> you can change it with compile flags.
> 
> I would put this one on a separate line, outside the comment.  It's
> minor, still we use this style in pg_config_manual.h.  See around
> REALLOCATE_BITMAPSETS.
> --
> Michael

+1

--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

> On Oct 27, 2025, at 20:13, Aleksander Alekseev <aleksander@tigerdata.com> wrote:
>
> Hi Nathan,
>
>>> Personally I don't have a strong opinion on this. We can easily add an
>>> exception for "/* ---" and "/* ===" comments if somebody believes this
>>> is a problem. I choose not to add such an exception just yet only
>>> because I don't like unnecessary exceptions :)
>>
>> +1 for adding an exception for "/* -----" style comments.  I tested running
>> pgindent after applying the patch, and the first thing I noticed was all
>> these (IMHO) unnecessary changes.  I don't think it helps readability, and
>> even if it did, it's arguably not worth the churn.
>
> OK, here is the corrected patch v5.
>
> --
> Best regards,
> Aleksander Alekseev
> <v5-0001-pgindent-improve-formatting-of-multiline-comments.patch>

1. I just ran the patched pgindent against a random file, then I got a lot diffs like:

```
        /*
-        * Direct advancement: avoid waking non-caught up backends that
-        * aren't interested in our notifications.
+        * Direct advancement: avoid waking non-caught up backends that aren't
+        * interested in our notifications.
         */
```

I am afraid that would generate a lot of noises for future reviews.

2. A typo in the patch

```
+    # Check each line except for the fist and the last one
```

fist => first

3. As you are updating pgindent, I want to report an issue, you may address in a separate patch or just in this patch,
upto you. 

See this code:
```
            else
                 /*
                  * fetch all the rest of the page
                  */
                 copysize = QUEUE_PAGESIZE - curoffset;
```

In the “else” clause, there is a multiple-line comment block, and a single line of code. Pgindent will add an empty
linebetween “else” and the comment block, which is weird. If the comment is one-line, then no empty line will be
inserted.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/