Thread
Commits
-
Improve indentation of multiline initialization expressions.
- 064750af4f4e 16.0 landed
-
pgindent vs variable declaration across multiple lines
Andres Freund <andres@anarazel.de> — 2023-01-20T01:31:37Z
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 -
Re: pgindent vs variable declaration across multiple lines
Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-20T01:43:44Z
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
-
Re: pgindent vs variable declaration across multiple lines
Andres Freund <andres@anarazel.de> — 2023-01-20T01:59:49Z
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 -
Re: pgindent vs variable declaration across multiple lines
Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-20T02:07:14Z
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
-
Re: pgindent vs variable declaration across multiple lines
Andres Freund <andres@anarazel.de> — 2023-01-20T23:12:01Z
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 -
Re: pgindent vs variable declaration across multiple lines
Robert Haas <robertmhaas@gmail.com> — 2023-01-20T23:50:56Z
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
-
Re: pgindent vs variable declaration across multiple lines
Thomas Munro <thomas.munro@gmail.com> — 2023-01-21T18:57:15Z
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... -
Re: pgindent vs variable declaration across multiple lines
Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-22T22:34:52Z
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
-
Re: pgindent vs variable declaration across multiple lines
Andres Freund <andres@anarazel.de> — 2023-01-22T22:40:34Z
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 <thomas.munro@gmail.com> — 2023-01-22T23:47:01Z
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.
-
Re: pgindent vs variable declaration across multiple lines
Andrew Dunstan <andrew@dunslane.net> — 2023-01-24T15:20:30Z
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
Tom Lane <tgl@sss.pgh.pa.us> — 2023-01-24T15:33:35Z
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
-
Re: pgindent vs variable declaration across multiple lines
Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-12T18:24:31Z
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