Review - Patch for pg_bsd_indent: improve formatting of multiline comments

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

This is a review of the pgindent multiline comments patch:
/messages/by-id/attachment/189778/v6-0001-pgindent-improve-formatting-of-multiline-comments.patch

Note: Feature implementation was tested and passes. Code style/compliance verified through review. installcheck-world and documentation testing were not performed (neither required for this patch type per PostgreSQL conventions for tool-only changes).

Contents & Purpose
==================
This patch adds a `postprocess_multiline_comment()` function to pgindent
that reformats multiline comments to put `/*` on its own line:

Before: /* This is line 1
* This is line 2
*/

After: /*
* This is line 1
* This is line 2
*/

The patch correctly excludes Doxygen (`/**`), compat flag (`/*-`), and
separator patterns (`/* ===`, `/* ---`).

Initial Run
===========
The patch applies cleanly to HEAD. I tested with various comment styles
and all cases passed:

- Basic multiline: reformatted correctly
- Doxygen/compat: left unchanged (correct)
- Separator lines: preserved (correct)
- Single-line comments: left unchanged (correct)
- Already-correct format: left unchanged (correct)

The validation logic at line 303 correctly requires `/*` on its own
line and ` */` on its own line before processing. Comments that don't
match this pattern are returned unchanged.

Performance
===========
Tested on a 74KB C file: 0.038 seconds. Negligible impact; the regex
only runs on comments.

Nitpicking & Conclusion
=======================
Minor: The regex notation in the report (line 85) shows `!` as part of
the pattern when it's actually the delimiter. The code itself is correct.

The validation description could be clearer that it returns early (skips
processing) when conditions aren't met, rather than "requires" them.

Otherwise this is a clean, minimal patch (36 lines) that fits well with
existing pgindent patterns. Single file change, no build system
modifications, developer tool only.

Hi Aleksander,

I tested v7 of the patch on current HEAD.

The patch applied cleanly and multiline comments were reformatted
correctly in my testing. I also verified that repeated pgindent runs
did not produce additional changes, and git diff --check was clean.

While testing some real PostgreSQL source files, I noticed
banner-style comments in contrib/seg/seg.c still receive formatting
changes like:

- This file contains routines ...

+ * This file contains routines ...

This looks similar to the earlier discussion around separator-style
comments and possible unnecessary diff churn. Since these header
comments already appear visually structured, perhaps preserving them
could help reduce additional formatting noise.

Apart from that observation, the patch looked good overall in my testing.

Regards,
solai murugan

solaimurugan vellaipandiyan <drsolaimurugan.v@gmail.com> writes:

While testing some real PostgreSQL source files, I noticed
banner-style comments in contrib/seg/seg.c still receive formatting
changes like:

-  This file contains routines ...
+ *  This file contains routines ...

This looks similar to the earlier discussion around separator-style
comments and possible unnecessary diff churn. Since these header
comments already appear visually structured, perhaps preserving them
could help reduce additional formatting noise.

I think those changes are fine; the point of this patch is to
standardize our multiline comments, not to allow multiple styles
to persist. In fact, if anything I think the v7 patch isn't going
far enough. I wondered why it's making this exception:

+	# Only format comments that match the expected format,
+	# or at least that could have been the author's intent.
+	if (   ($lines[0] ne "/*" && $lines[-1] ne " */")
+		or ($lines[1] !~ m!^\s+\*!))
+	{
+		return $source;
+	}

I tried removing that, and soon found that it's the only thing
stopping the code from converting

/* foo bar */

to

/*
* foo bar
*/

which I don't think we want. So instead I replaced that bit with
an explicit test preventing reformatting a single-line comment.
After that it made a lot of changes that I thought were improvements,
but there were still some places where the output wasn't very uniform,
so I did some additional hacking to normalize the leading whitespace
and the number of '*' characters.

Attached are a proposed v8 of the patch, plus two diff files showing
the effects. v7-0001.diff.nocfbot is what the v7 patch does with
today's HEAD (it's the same as before). The v8 patch makes all those
changes and in addition makes the ones shown in v8-0001.diff.nocfbot.
I think those are pretty much all improvements, except that it kind
of messes up Martin Utesch's ASCII-art signatures in the geqo files.
That's because there are some lines starting with '*' and some with
'='. This is another place where I doubt it's worth the trouble to
try to make pgindent handle the case nicely; I propose just manually
adding leading '*'s to those comments before running pgindent.

regards, tom lane

On Mon, May 11, 2026 at 6:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Attached are a proposed v8 of the patch, plus two diff files showing
the effects. v7-0001.diff.nocfbot is what the v7 patch does with
today's HEAD (it's the same as before). The v8 patch makes all those
changes and in addition makes the ones shown in v8-0001.diff.nocfbot.
I think those are pretty much all improvements, except that it kind
of messes up Martin Utesch's ASCII-art signatures in the geqo files.

Agreed, although this one now seems to use even more vertical space
that doesn't contain anything:

-/* Function works as follows:
+/*
+ * Function works as follows:
+ *
  *
  *
- * */
+ */

--
John Naylor
Amazon Web Services

John Naylor <johncnaylorls@gmail.com> writes:

On Mon, May 11, 2026 at 6:37 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think those are pretty much all improvements, except that it kind
of messes up Martin Utesch's ASCII-art signatures in the geqo files.

Agreed, although this one now seems to use even more vertical space
that doesn't contain anything:

-/* Function works as follows:
+/*
+ * Function works as follows:
+ *
*
*
- * */
+ */

Yeah ... what's actually happening there is that it's replacing

* */

with

*
*/

although the diff misattributes the extra line. There are few
enough of those that I didn't think it was worth getting
excited about ...

regards, tom lane

Hi Tom,

I tested the proposed v8 patch on current HEAD.

I reran pgindent on files like contrib/seg/seg.c and
src/port/inet_aton.c, and the updated formatting looked more
consistent overall.
I also checked that repeated pgindent runs did not introduce
additional changes, and git diff --check was clean.
Additionally, the single-line comment case now stays unchanged as expected.
Overall, the new normalization behavior looks much cleaner and more
uniform in my testing.

Regards,
Solaimurugan V

Hi,

This is a review of the pgindent multiline comments patch:

/messages/by-id/attachment/189778/v6-0001-pgindent-improve-formatting-of-multiline-comments.patch

Thanks for the feedback. In the future please submit your code reviews
to the corresponding thread. You created a new one so anyone following
the original thread [1]/messages/by-id/CAJ7c6TPQ0kkHQG-AqeAJ3PV_YtmDzcc7s+_V4=t+xgSnZm1cFw@mail.gmail.com[2]https://commitfest.postgresql.org/patch/5831/ has a good chance to miss this discussion.

I suggest continuing the discussion in [1]/messages/by-id/CAJ7c6TPQ0kkHQG-AqeAJ3PV_YtmDzcc7s+_V4=t+xgSnZm1cFw@mail.gmail.com.

[1]: /messages/by-id/CAJ7c6TPQ0kkHQG-AqeAJ3PV_YtmDzcc7s+_V4=t+xgSnZm1cFw@mail.gmail.com
[2]: https://commitfest.postgresql.org/patch/5831/

--
Best regards,
Aleksander Alekseev

On 2026-May-10, Tom Lane wrote:

Attached are a proposed v8 of the patch, plus two diff files showing
the effects. v7-0001.diff.nocfbot is what the v7 patch does with
today's HEAD (it's the same as before). The v8 patch makes all those
changes and in addition makes the ones shown in v8-0001.diff.nocfbot.
I think those are pretty much all improvements, except that it kind
of messes up Martin Utesch's ASCII-art signatures in the geqo files.
That's because there are some lines starting with '*' and some with
'='. This is another place where I doubt it's worth the trouble to
try to make pgindent handle the case nicely; I propose just manually
adding leading '*'s to those comments before running pgindent.

Just passing by, but I think this comment should be handled manually as
well, as I doubt we want it to end up this way:

diff --git a/src/backend/utils/adt/tsrank.c b/src/backend/utils/adt/tsrank.c
index d35e5528d0a..6b4bf8eb13b 100644
--- a/src/backend/utils/adt/tsrank.c
+++ b/src/backend/utils/adt/tsrank.c
@@ -337,12 +337,12 @@ calc_rank_or(const float *w, TSVector t, TSQuery q)
}
}
/*
-			limit (sum(1/i^2),i=1,inf) = pi^2/6
-			resj = sum(wi/i^2),i=1,noccurrence,
-			wi - should be sorted desc,
-			don't sort for now, just choose maximum weight. This should be corrected
-			Oleg Bartunov
-*/
+ * limit (sum(1/i^2),i=1,inf) = pi^2/6
+ * resj = sum(wi/i^2),i=1,noccurrence,
+ * wi - should be sorted desc,
+ * don't sort for now, just choose maximum weight. This should be corrected
+ * Oleg Bartunov
+ */
res = res + (wjm + resj - wjm / ((jm + 1) * (jm + 1))) / 1.64493406685;

entry++;

(Not that I understand what this is trying to tell me, mind)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:

Just passing by, but I think this comment should be handled manually as
well, as I doubt we want it to end up this way:

Hmmm ... yeah, probably. In isolation the diff had looked all right
to me, but it would probably make more sense to keep this comment
aligned with the surrounding code. I'm thinking like

}
/*-----
* limit (sum(1/i^2),i=1,inf) = pi^2/6
* resj = sum(wi/i^2),i=1,noccurrence,
* wi - should be sorted desc,
* don't sort for now, just choose maximum weight.
* This should be corrected
* Oleg Bartunov
*/
res = res + (wjm + resj - wjm / ((jm + 1) * (jm + 1))) / 1.64493406685;

(Not that I understand what this is trying to tell me, mind)

Me either :-(

regards, tom lane

Hi,

/*-----
* limit (sum(1/i^2),i=1,inf) = pi^2/6
* resj = sum(wi/i^2),i=1,noccurrence,
* wi - should be sorted desc,
* don't sort for now, just choose maximum weight.
* This should be corrected
* Oleg Bartunov
*/
res = res + (wjm + resj - wjm / ((jm + 1) * (jm + 1))) / 1.64493406685;

(Not that I understand what this is trying to tell me, mind)

Me either :-(

I *think* maybe I was able to decipher it. PFA the patch.

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@tigerdata.com> writes:

(Not that I understand what this is trying to tell me, mind)

Me either :-(

I *think* maybe I was able to decipher it. PFA the patch.

Yeah, that seems like a plausible reconstruction of the logic.
I went ahead and pushed it.

Attached are the proposed pgindent change (same as before),
the additional manual corrections we've discussed needing,
and then the results of running the new pgindent after.
This looks pretty ready-to-go to me.

regards, tom lane