Обсуждение: pgindent versus struct members and typedefs
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
> On Dec 3, 2025, at 06:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> 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.
>
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
statementwithout 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/
Chao Li <li.evan.chao@gmail.com> writes:
> On Dec 3, 2025, at 06:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> 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.
> I think that’s fine.
Agreed, not worth the trouble to fool with.
> Actually I see the other problem with pgindent, where if a “else” clause contains a multiple-line comment and a
singlestatement without braces, for example:
> ...
> I tried to fix but failed. For that problem, a solution is to add braces to the “else” clause.
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
> On Dec 3, 2025, at 06:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Chao Li <li.evan.chao@gmail.com> writes:
>> On Dec 3, 2025, at 06:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>>> 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.
>
>> I think that’s fine.
>
> Agreed, not worth the trouble to fool with.
>
>> Actually I see the other problem with pgindent, where if a “else” clause contains a multiple-line comment and a
singlestatement without braces, for example:
>> ...
>> I tried to fix but failed. For that problem, a solution is to add braces to the “else” clause.
>
> 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.
>
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,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Chao Li <li.evan.chao@gmail.com> writes:
>> On Dec 3, 2025, at 06:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.
> 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
> On Dec 3, 2025, at 07:13, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Chao Li <li.evan.chao@gmail.com> writes: >>> On Dec 3, 2025, at 06:51, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 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. > >> 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. > Totally agreed. In my first job at Lucent Technologies, the coding standard was that braces should always be added even ifa clause has only one line of code. I remember one of the explanations was like, if braces has been added, then later whena new line of code is added to the clause, there is only one line of diff, otherwise braces need to be added, so it wouldbe 3 lines of diffs. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
On 2025-12-02 Tu 6:31 PM, Chao Li wrote:
On Dec 3, 2025, at 07:13, Tom Lane <tgl@sss.pgh.pa.us> wrote: Chao Li <li.evan.chao@gmail.com> writes:On Dec 3, 2025, at 06:51, Tom Lane <tgl@sss.pgh.pa.us> wrote: 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.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.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.
+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
On 2025-Dec-03, Andrew Dunstan 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. 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)