Re: pgindent vs variable declaration across multiple lines
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 diff --git a/src/tools/pg_bsd_indent/args.c b/src/tools/pg_bsd_indent/args.c index d08b086a88..38eaa5a5bf 100644 --- a/src/tools/pg_bsd_indent/args.c +++ b/src/tools/pg_bsd_indent/args.c @@ -51,7 +51,7 @@ static char sccsid[] = "@(#)args.c 8.1 (Berkeley) 6/6/93"; #include "indent_globs.h" #include "indent.h" -#define INDENT_VERSION "2.1.1" +#define INDENT_VERSION "2.1.2" /* profile types */ #define PRO_SPECIAL 1 /* special case */ diff --git a/src/tools/pg_bsd_indent/io.c b/src/tools/pg_bsd_indent/io.c index 4149424294..9d64ca1ee5 100644 --- a/src/tools/pg_bsd_indent/io.c +++ b/src/tools/pg_bsd_indent/io.c @@ -201,11 +201,12 @@ dump_line(void) ps.decl_on_line = ps.in_decl; /* if we are in the middle of a * declaration, remember that fact for * proper comment indentation */ -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 */ +/* next line should be indented if we have not completed this stmt, and + * either we are not in a declaration or we are in an initialization + * assignment; but not if we're within braces in an initialization, + * because that scenario is handled by other rules. */ +ps.ind_stmt = ps.in_stmt && + (!ps.in_decl || (ps.block_init && ps.block_init_level <= 0)); ps.use_ff = false; ps.dumped_decl_indent = 0; *(e_lab = s_lab) = '\0'; /* reset buffers */ diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index a793971e07..3398d62133 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -13,7 +13,7 @@ use IO::Handle; use Getopt::Long; # Update for pg_bsd_indent version -my $INDENT_VERSION = "2.1.1"; +my $INDENT_VERSION = "2.1.2"; # Our standard indent settings my $indent_opts =
Re: pgindent vs variable declaration across multiple lines
Andrew Dunstan 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
Re: pgindent vs variable declaration across multiple lines
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
Re: pgindent vs variable declaration across multiple lines
On Mon, Jan 23, 2023 at 11:34 AM 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. 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.
Re: pgindent vs variable declaration across multiple lines
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
Re: pgindent vs variable declaration across multiple lines
Thomas Munro writes:
> On Fri, Jan 20, 2023 at 2:43 PM Tom Lane 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
diff --git a/io.c b/io.c
index 3ce8bfb..8a05d7e 100644
--- a/io.c
+++ b/io.c
@@ -205,11 +205,12 @@ dump_line(void)
ps.decl_on_line = ps.in_decl; /* if we are in the middle of a
* declaration, remember that fact for
* proper comment indentation */
-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 */
+/* next line should be indented if we have not completed this stmt, and
+ * either we are not in a declaration or we are in an initialization
+ * assignment; but not if we're within braces in an initialization,
+ * because that scenario is handled by other rules. */
+ps.ind_stmt = ps.in_stmt &&
+ (!ps.in_decl || (ps.block_init && ps.block_init_level <= 0));
ps.use_ff = false;
ps.dumped_decl_indent = 0;
*(e_lab = s_lab) = '\0'; /* reset buffers */
diff --git a/contrib/ltree/ltree_gist.c b/contrib/ltree/ltree_gist.c
index 5d2db6c62b..119d03522f 100644
--- a/contrib/ltree/ltree_gist.c
+++ b/contrib/ltree/ltree_gist.c
@@ -43,7 +43,7 @@ ltree_gist_alloc(bool isalltrue, BITVECP sign, int siglen,
ltree *left, ltree *right)
{
int32 size = LTG_HDRSIZE + (isalltrue ? 0 : siglen) +
- (left ? VARSIZE(left) + (right ? VARSIZE(right) : 0) : 0);
+ (left ? VARSIZE(left) + (right ? VARSIZE(right) : 0) : 0);
ltree_gist *result = palloc(size);
SET_VARSIZE(result, size);
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index e523d22eba..295c7dcc22 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -290,7 +290,7 @@ pg_decode_begin_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
TestDecodingData *data = ctx->output_plugin_private;
TestDecodingTxnData *txndata =
- MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
+ MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
txndata->xact_wrote_changes = false;
txn->output_plugin_private = txndata;
@@ -350,7 +350,7 @@ pg_decode_begin_prepare_txn(LogicalDecodingContext *ctx, ReorderBufferTXN *txn)
{
TestDecodingData *data = ctx->output_plugin_private;
TestDecodingTxnData *txndata =
- MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
+ MemoryContextAllocZero(ctx->context, sizeof(TestDecodingTxnData));
txndata->xact_wrote_changes = false;
txn->output_plugin_private = txndata;
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 14c23101ad..dfcb4d279b 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1717,7
Re: pgindent vs variable declaration across multiple lines
On Fri, Jan 20, 2023 at 2:43 PM Tom Lane wrote: > Andres Freund 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...
Re: pgindent vs variable declaration across multiple lines
On Thu, Jan 19, 2023 at 8:31 PM Andres Freund 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
Re: pgindent vs variable declaration across multiple lines
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 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
Re: pgindent vs variable declaration across multiple lines
Andres Freund 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
Re: pgindent vs variable declaration across multiple lines
Hi, On 2023-01-19 20:43:44 -0500, Tom Lane wrote: > Andres Freund 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
Re: pgindent vs variable declaration across multiple lines
Andres Freund 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
pgindent vs variable declaration across multiple lines
Hi, 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); even though the same pattern works, and is used fairly widely for assignments amroutine->amparallelvacuumoptions = VACUUM_OPTION_PARALLEL_BULKDEL; Particularly when variable and/or types names are longer, it's sometimes hard to fit enough into one line to use a different style. E.g., the code I'm currently hacking on has RWConflict possibleUnsafeConflict = dlist_container(RWConflictData, inLink, iter.cur); There's simply no way to make break that across lines that doesn't either violate the line length limit or makes pgindent do odd things: too long line: RWConflict possibleUnsafeConflict = dlist_container(RWConflictData, inLink, iter.cur); pgindent will move start of second line: RWConflict possibleUnsafeConflict = dlist_container(RWConflictData, inLink, iter.cur); I know I can leave the variable initially uninitialized and then do a separate assignment, but that's not a great fix. And sometimes other initializations want to access the variable alrady. Do others dislike this as well? I assume we'd again have to dive into pg_bsd_indent's code to fix it :( And even if we were to figure out how, would it be worth the reindent-all-branches pain? I'd say yes, but... Greetings, Andres Freund
