pgindent vs variable declaration across multiple lines

Andres Freund <andres@anarazel.de> writes:

There's a few places in the code that try to format a variable definition like this

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

but pgindent turns that into

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

Yeah, that's bugged me too. I suspect that the triggering factor is
use of a typedef name within the assigned expression, but I've not
tried to run it to ground.

I assume we'd again have to dive into pg_bsd_indent's code to fix it :(

Yeah :-(. That's enough of a rat's nest that I've not really wanted to.
But I'd support applying such a fix if someone can figure it out.

And even if we were to figure out how, would it be worth the
reindent-all-branches pain? I'd say yes, but...

What reindent-all-branches pain? We haven't done an all-branches
reindent in the past, even for pgindent fixes that touched far more
code than this would (assuming that the proposed fix doesn't have
other side-effects).

regards, tom lane

Hi,

On 2023-01-19 20:43:44 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

There's a few places in the code that try to format a variable definition like this

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

but pgindent turns that into

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

Yeah, that's bugged me too. I suspect that the triggering factor is
use of a typedef name within the assigned expression, but I've not
tried to run it to ground.

It's not that - it happens even with just
int frak =
1;

since it doesn't happen for plain assignments, I think it's somehow related to
code dealing with variable declarations.

I assume we'd again have to dive into pg_bsd_indent's code to fix it :(

Yeah :-(. That's enough of a rat's nest that I've not really wanted to.
But I'd support applying such a fix if someone can figure it out.

It's pretty awful code :(

And even if we were to figure out how, would it be worth the
reindent-all-branches pain? I'd say yes, but...

What reindent-all-branches pain? We haven't done an all-branches
reindent in the past, even for pgindent fixes that touched far more
code than this would (assuming that the proposed fix doesn't have
other side-effects).

Oh. I thought we had re-indented the other branches when we modified
pg_bsd_intent substantially in the past, to reduce backpatching pain. But I
guess we just discussed that option, but didn't end up pursuing it.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2023-01-19 20:43:44 -0500, Tom Lane wrote:

What reindent-all-branches pain? We haven't done an all-branches
reindent in the past, even for pgindent fixes that touched far more
code than this would (assuming that the proposed fix doesn't have
other side-effects).

Oh. I thought we had re-indented the other branches when we modified
pg_bsd_intent substantially in the past, to reduce backpatching pain. But I
guess we just discussed that option, but didn't end up pursuing it.

Yeah, we did discuss it, but never did it --- I think the convincing
argument not to was that major reformatting would be very painful
for people maintaining forks, and we shouldn't put them through that
to track minor releases.

regards, tom lane

Hi,

On 2023-01-19 17:59:49 -0800, Andres Freund wrote:

On 2023-01-19 20:43:44 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

There's a few places in the code that try to format a variable definition like this

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

but pgindent turns that into

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

Yeah, that's bugged me too. I suspect that the triggering factor is
use of a typedef name within the assigned expression, but I've not
tried to run it to ground.

It's not that - it happens even with just
int frak =
1;

since it doesn't happen for plain assignments, I think it's somehow related to
code dealing with variable declarations.

Another fun one: pgindent turns

return (instr_time) {t.QuadPart};
into
return (struct instr_time)
{
t.QuadPart
};

Obviously it can be dealt with with a local variable, but ...

Greetings,

Andres Freund

On Thu, Jan 19, 2023 at 8:31 PM Andres Freund <andres@anarazel.de> wrote:

I know I can leave the variable initially uninitialized and then do a separate
assignment, but that's not a great fix.

That's what I do.

If you pick names for all of your data types that are very very long
and wordy then you don't feel as bad about this, because you were
gonna need a line break anyway. :-)

--
Robert Haas
EDB: http://www.enterprisedb.com

On Fri, Jan 20, 2023 at 2:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

There's a few places in the code that try to format a variable definition like this

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

but pgindent turns that into

ReorderBufferChange *next_change =
dlist_container(ReorderBufferChange, node, next);

Yeah, that's bugged me too. I suspect that the triggering factor is
use of a typedef name within the assigned expression, but I've not
tried to run it to ground.

I assume we'd again have to dive into pg_bsd_indent's code to fix it :(

Yeah :-(. That's enough of a rat's nest that I've not really wanted to.
But I'd support applying such a fix if someone can figure it out.

This may be a clue: the place where declarations are treated
differently seems to be (stangely) in io.c:

ps.ind_stmt = ps.in_stmt & ~ps.in_decl; /* next line should be
* indented if we have not
* completed this stmt and if
* we are not in the middle of
* a declaration */

If you just remove "& ~ps.in_decl" then it does the desired thing for
that new code in predicate.c, but it also interferes with declarations
with commas, ie int i, j; where i and j currently line up, now j just
gets one indentation level. It's probably not the right way to do it
but you can fix that with a last token kluge, something like:

#include "indent_codes.h"

ps.ind_stmt = ps.in_stmt && (!ps.in_decl || ps.last_token != comma);

That improves a lot of code in our tree IMHO but of course there is
other collateral damage...

Thomas Munro <thomas.munro@gmail.com> writes:

On Fri, Jan 20, 2023 at 2:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Yeah :-(. That's enough of a rat's nest that I've not really wanted to.
But I'd support applying such a fix if someone can figure it out.

This may be a clue: the place where declarations are treated
differently seems to be (stangely) in io.c:

ps.ind_stmt = ps.in_stmt & ~ps.in_decl; /* next line should be
* indented if we have not
* completed this stmt and if
* we are not in the middle of
* a declaration */

If you just remove "& ~ps.in_decl" then it does the desired thing for
that new code in predicate.c, but it also interferes with declarations
with commas, ie int i, j; where i and j currently line up, now j just
gets one indentation level. It's probably not the right way to do it
but you can fix that with a last token kluge, something like:
#include "indent_codes.h"
ps.ind_stmt = ps.in_stmt && (!ps.in_decl || ps.last_token != comma);
That improves a lot of code in our tree IMHO but of course there is
other collateral damage...

I spent some more time staring at this and came up with what seems like
a workable patch, based on the idea that what we want to indent is
specifically initialization expressions. pg_bsd_indent does have some
understanding of that: ps.block_init is true within such an expression,
and then ps.block_init_level is the brace nesting depth inside it.
If you just enable ind_stmt based on block_init then you get a bunch
of unwanted additional indentation inside struct initializers, but
it seems to work okay if you restrict it to not happen inside braces.
More importantly, it doesn't change anything we don't want changed.

Proposed patch for pg_bsd_indent attached. I've also attached a diff
representing the delta between what current pg_bsd_indent wants to do
to HEAD and what this would do. All the changes it wants to make look
good, although I can't say whether there are other places it's failing
to change that we'd like it to.

regards, tom lane

Hi,

On 2023-01-22 17:34:52 -0500, Tom Lane wrote:

I spent some more time staring at this and came up with what seems like
a workable patch, based on the idea that what we want to indent is
specifically initialization expressions.

That's awesome. Thanks for doing that.

Proposed patch for pg_bsd_indent attached. I've also attached a diff
representing the delta between what current pg_bsd_indent wants to do
to HEAD and what this would do. All the changes it wants to make look
good, although I can't say whether there are other places it's failing
to change that we'd like it to.

I think it's a significant improvement, even if it turns out that there's
other cases it misses.

Greetings,

Andres Freund

On Mon, Jan 23, 2023 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I spent some more time staring at this and came up with what seems like
a workable patch, based on the idea that what we want to indent is
specifically initialization expressions. pg_bsd_indent does have some
understanding of that: ps.block_init is true within such an expression,
and then ps.block_init_level is the brace nesting depth inside it.
If you just enable ind_stmt based on block_init then you get a bunch
of unwanted additional indentation inside struct initializers, but
it seems to work okay if you restrict it to not happen inside braces.
More importantly, it doesn't change anything we don't want changed.

Nice! LGTM now that I know about block_init.

On 2023-01-22 Su 17:34, Tom Lane wrote:

I've also attached a diff
representing the delta between what current pg_bsd_indent wants to do
to HEAD and what this would do. All the changes it wants to make look
good, although I can't say whether there are other places it's failing
to change that we'd like it to.

Changes look good. There are a handful of places where I think the code
would be slightly more readable if a leading typecast were moved to the
second line.

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:

On 2023-01-22 Su 17:34, Tom Lane wrote:

I've also attached a diff
representing the delta between what current pg_bsd_indent wants to do
to HEAD and what this would do. All the changes it wants to make look
good, although I can't say whether there are other places it's failing
to change that we'd like it to.

Changes look good. There are a handful of places where I think the code
would be slightly more readable if a leading typecast were moved to the
second line.

Possibly, but that's the sort of decision that pgindent leaves to human
judgment I think. It'll reflow comment blocks across lines, but I don't
recall having seen it move line breaks within code.

regards, tom lane

Now that pg_bsd_indent is in our tree, we can format this as a
patch against Postgres sources. I'll stick it in the March CF
so we don't forget about it.

regards, tom lane