Subject: pgindent versus struct members and typedefs

From: Nathan Bossart <[email-masked]>
Date: Tue, 2 Dec 2025 16:00:51 -0600

On Mon, Dec 01, 2025 at 05:04:23PM -0600, Nathan Bossart wrote:
> On Tue, Dec 02, 2025 at 05:35:34AM +0800, Chao Li wrote:
>> ```
>> +		else if (entry->type == DSMR_ENTRY_TYPE_DSH &&
>> +				 entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)
>> ```
>> 
>> Missing a white space after !=.
> 
> I agree, but for some reason, pgindent insists on removing that space.  I'm
> leaving that for another thread.

So, this seems to have something to do with the struct member having the
same name as a typedef.  If I rename the member, pgindent adds the space as
expected.  Interestingly, changing the != to a == also fixes the spacing.
There are a couple of other examples in the code:

src/backend/storage/ipc/dsm_registry.c:				 entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)
src/backend/replication/logical/logicalfuncs.c:			ctx->options.output_type !=OUTPUT_PLUGIN_TEXTUAL_OUTPUT)
src/bin/pg_basebackup/pg_basebackup.c:	if (state.manifest_file !=NULL)
src/bin/pg_basebackup/pg_basebackup.c:					state->manifest_file !=NULL)
src/bin/pg_basebackup/pg_basebackup.c:				else if (state->manifest_file !=NULL)

I used the following command to find these:

	grep -E "!=[A-Za-z]" ./* -rI

AFAICT this is a special case of the note added to pgindent's README by
commit c4133ec:

	pgindent will mangle both declaration and definition of a C function whose
	name matches a typedef.  Currently the best workaround is to choose
	non-conflicting names.

I tried to fix pgindent for a few, but the code is basically impenetrable.
I didn't find any fixes upstream [0], either.  As noted above, we could
also fix it by avoiding the naming conflicts.  However, I can't imagine
that's worth the churn, and I've already spent way too much time on this,
so IMHO the best thing to do here is nothing.

[0] https://github.com/freebsd/freebsd-src/tree/main/usr.bin/indent

-- 
nathan

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 06:46:33 +0800

> On Dec 3, 2025, at 06:00, Nathan Bossart <[email-masked]> wrote:
> 

I think that’s fine.

Actually I see the other problem with pgindent, where if a “else” clause contains a multiple-line comment and a single statement without braces, for example:

```
else
     /*
      * comment
      */
     printf(…);
```

Then pgindent will blindly add an empty line after “else”, so we get:
```
else

     /*
      * comment
      */
     printf(…);

```

I tried to fix but failed. For that problem, a solution is to add braces to the “else” clause.

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

---

From: Tom Lane <[email-masked]>
Date: Tue, 02 Dec 2025 17:51:15 -0500

> I think that’s fine.

Agreed, not worth the trouble to fool with.

> ...

In this case, I think pgindent is indirectly enforcing good style.
I do not like omitting braces around anything that's more than one
line; readers have to pay close attention to whether the code is
doing what it was intended to.

			regards, tom lane

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 07:06:45 +0800

> On Dec 3, 2025, at 06:51, Tom Lane <[email-masked]> wrote:
> 

For “one line”, do you mean only a single line of statement or one line statement plus one line comment?

To clarify the pgindnet problem, if we have a one-line comment plus one-line statement, for example:
```
    else
          /* one line comment */
          printf(…);
```

In this case, pgindent will not add an empty line after “else”.

But I totally agree with you, when there is a multiple-line comment and a single statement, it's a good habit to add braces.

Best regards,

---

From: Tom Lane <[email-masked]>
Date: Tue, 02 Dec 2025 18:13:16 -0500

> For “one line”, do you mean only a single line of statement or one line statement plus one line comment?

In my head, a comment and a statement are two lines, and so need
wrapping braces as much as two statements would do.  I realize that
C compilers think differently, but for readability and modifiability
reasons that's the approach I take.

			regards, tom lane

---

From: Chao Li <[email-masked]>
Date: Wed, 3 Dec 2025 07:31:48 +0800

> On Dec 3, 2025, at 07:13, Tom Lane <[email-masked]> wrote:
> 

Totally agreed. In my first job at Lucent Technologies, the coding standard was that braces should always be added even if a clause has only one line of code. I remember one of the explanations was like, if braces has been added, then later when a new line of code is added to the clause, there is only one line of diff, otherwise braces need to be added, so it would be 3 lines of diffs.

Best regards,

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 3 Dec 2025 10:35:53 -0500

On 2025-12-02 Tu 6:31 PM, Chao Li wrote:
>
>> On Dec 3, 2025, at 07:13, Tom Lane<[email-masked]> wrote:
>>
>> Chao Li<[email-masked]> writes:
>>>> On Dec 3, 2025, at 06:51, Tom Lane<[email-masked]> wrote:

+1. One of the things I find particularly un-aesthetic is having some 
branches of an if statement with braces and some without. We have lots 
of cases of that, but I try to avoid it.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

---

From: Álvaro Herrera <[email-masked]>
Date: Wed, 3 Dec 2025 18:25:27 +0100

On 2025-Dec-03, Andrew Dunstan wrote:

> branches of an if statement with braces and some without. We have lots of
> cases of that, but I try to avoid it.

I actually prefer that style: when there are only two branches, and the
other branch of the if is long, I put the short one first without
braces, and then braces around the long "else" one.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 5 May 2026 16:47:07 -0500

> Agreed, not worth the trouble to fool with.

For fun, I spent some time with an AI tool to develop the attached fix for
this problem.  The explanation seems reasonable to me, although I am by no
means a pgindent expert.  When I looked at this in December, I did find
this similar commit from upstream [0], but I failed to make the connection
with last_u_d.  0002 is the result of a pgindent run after applying 0001.
You'll notice that it fixes the exact set of cases I found with grep
upthread.

[0] https://github.com/pstef/freebsd_indent/commit/afa2239

-- 
nathan

---

From: Chao Li <[email-masked]>
Date: Wed, 6 May 2026 09:29:36 +0800

> On May 6, 2026, at 05:47, Nathan Bossart <[email-masked]> wrote:
> 
> <v1-0001-pgindent-Fix-spacing-after-when-member-name-match.patch><v1-0002-run-pgindent.patch>

From 0002, the fix looks good. I tried to run the patched pgindent against all .c and .h files under src/ and contrib/, the result is exactly the same as 0002.

So, maybe worthy pushing before Tom running the annual pgindent.

Best regards,

---

From: Tom Lane <[email-masked]>
Date: Tue, 05 May 2026 23:43:39 -0400

> You'll notice that it fixes the exact set of cases I found with grep

Those changes are clearly improvements.  I'm too tired to investigate
right now, but I wonder if we should adopt the upstream fix you
mention?  (Or more generally, other changes they made since we forked?)

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 6 May 2026 09:44:26 -0500

> mention?  (Or more generally, other changes they made since we forked?)

The upstream fix is from before we forked, it just didn't fix this
particular case.  I don't see any missing changes from
pstef/freebsd_indent, but there have been a number of changes in the
FreeBSD version:

    https://cgit.freebsd.org/src/log/usr.bin/indent

Some of our changes to pg_bsd_indent bumped INDENT_VERSION.  Should we do
that here?

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 06 May 2026 11:02:35 -0400

> Some of our changes to pg_bsd_indent bumped INDENT_VERSION.  Should we do

We already have an INDENT_VERSION bump queued for the
space-between-comma-and-period change.  I don't think we need two
bumps in this cycle, as long as we coordinate pushing these changes.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 6 May 2026 10:15:07 -0500

> bumps in this cycle, as long as we coordinate pushing these changes.

Okay.  I'll go ahead and commit the patches for $subject then, leaving out
the version bump.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 06 May 2026 11:17:17 -0400

> Okay.  I'll go ahead and commit the patches for $subject then, leaving out

No, *don't* do it right now.  You'll break anyone using the
in-tree version, or if you also commit the ensuing code changes,
you'll break anyone using an out-of-tree copy.  This stuff all
needs to go in at once.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 6 May 2026 10:20:26 -0500

> you'll break anyone using an out-of-tree copy.  This stuff all

Alright.  I'll just prepare the patches and post them here for when that
time comes, then.

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 6 May 2026 11:13:21 -0500

> Alright.  I'll just prepare the patches and post them here for when that

Here's a new version of 0001 with a cleaned-up commit message.  I've
omitted 0002, since it's just the result of running pgindent after apply
the first one.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Tue, 12 May 2026 17:33:50 -0400

> omitted 0002, since it's just the result of running pgindent after apply

I think we're about ready to roll on doing the pgindent run.
Do you want to push your patch tomorrow AM, and then I'll get
on with the rest?

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 12 May 2026 17:09:00 -0500

> Do you want to push your patch tomorrow AM, and then I'll get

Sounds good.  We won't be back-patching any of this, right?

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Tue, 12 May 2026 18:12:21 -0400

> Sounds good.  We won't be back-patching any of this, right?

Right, just HEAD.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 13 May 2026 09:12:04 -0500

>> Sounds good.  We won't be back-patching any of this, right?

Committed.

-- 
nathan

---

From: Tom Lane <[email-masked]>
Date: Wed, 13 May 2026 10:13:58 -0400

> Committed.

Thanks, I'll start on the rest.

			regards, tom lane