Thread: Should we remove -Wdeclaration-after-statement?
Postgres currently requires all variables to be declared at the top of the function, because it specifies -Wdeclaration-after-statement. One of the reasons that we had this warning was because C89 required this style of declaration. Requiring it everywhere made backporting easier, since some of our older supported PG versions needed to compile on C89. Now that we have dropped support for PG11 that reason goes away, since now all supported Postgres versions require C99. So, I think it's worth reconsidering if we want this warning to be enabled or not. Personally I would very much prefer the warning to be disabled for the following reasons: 1. It allows Asserts, ereports and other checks at the top of a function, making it clear to the reader if there are any requirements on the arguments that the caller should uphold. 2. By declaring variables at their first usage you limit the scope of the variable. This reduces the amount of code that a reader of the code has to look at to see if the variable was changed between its declaration and the usage location that they are interested in. 3. By declaring variables at their first usage, often you can immediately see the type of the variable that you are looking at. Since most variables are not used in the whole function, their first usage is pretty much their only usage. 4. By declaring variables at their first usage, you can often initialize them with a useful value right away. That way you don't have to worry about using it uninitialized. It also reduces the lines of code, since initialization and declaration can be done in the same line. 5. By declaring variables at their first usage, it is made clear to the reader why the variable needs to exist. Oftentimes when I read a postgres function from top to bottom, it's unclear to me what purpose some of the declared variables at the top have. 6. I run into this warning over and over again when writing code in postgres. This is because all of the other programming languages I write code in don't have this restriction. Even many C projects I work in don't have this restriction on where variables can be declared.
Jelte Fennema-Nio <postgres@jeltef.nl> writes: > Postgres currently requires all variables to be declared at the top of > the function, because it specifies -Wdeclaration-after-statement. One > of the reasons that we had this warning was because C89 required this > style of declaration. Requiring it everywhere made backporting easier, > since some of our older supported PG versions needed to compile on > C89. Now that we have dropped support for PG11 that reason goes away, > since now all supported Postgres versions require C99. So, I think > it's worth reconsidering if we want this warning to be enabled or not. This has already been debated, and the conclusion was that we would stick to the existing style for consistency reasons. The fact that back-portable patches required it was only one of the arguments, and not the decisive one. regards, tom lane
On Wed, 27 Dec 2023 at 16:05, Tom Lane <tgl@sss.pgh.pa.us> wrote: > This has already been debated, and the conclusion was that we would > stick to the existing style for consistency reasons. I looked through the archives quite a bit, but I couldn't find any conclusive debate about the current declaration style. Definitely not one with "consistency reasons" as being the conclusion. Could you point me to the place where that conclusion was reached? Or could you at least clarify what consistency you believe is lost by removing the warning? The threads discussing this warning that I did find were the following: The initial addition of the warning flag[1], which has very little discussion. Introducing the C99 requirement[2]. Robert and you both preferred the current declaration style. Andrew and Andres both would want to accept the new declaration style. Another where removal of this warning was suggested[3], and where Andres said he was in favor of removing the warning. But he didn't think fighting for it was worth the effort at the time to fight you and Robert, when he was trying to get the general C99 requirement in. And finally, one that was started by me, where I suggest an automated refactor[4]. This change got shot down because it would cause lots of backpatching problems (and because it was using perl regexes instead of an AST parser to do the automated refactor). Ranier and you were proponents of the current declaration style. Chapman was in favor of the new declaration style. Andrew seems neutral. P.S. Note, that I'm not suggesting a complete refactor this time. I'm only proposing to relax the rules, and disable the warning, so newly written code can benefit. But if the only reason not to remove the warning is that then there would be two styles of declaration in the codebase, then I'm happy to create another refactoring script that moves declarations down to their first usage. (Which could then be run on all backbranches to make sure there is no backpatching pain) [1]: https://www.postgresql.org/message-id/flat/417263F8.4060102%40samurai.com [2]: https://www.postgresql.org/message-id/flat/CA%2BTgmoYvHzFkwChsamwbBrLNJRcRq%2BfyTwveFaN_YOWUsRnfpw%40mail.gmail.com#931f4c68237caf4c60b4dc298236aef1 [3]: https://www.postgresql.org/message-id/flat/20181213210012.i7iihamlbi7vfdiw%40alap3.anarazel.de#00304f9dfc039da87383fed30be62cff [4]: https://www.postgresql.org/message-id/flat/AM5PR83MB0178E68E4FF1BAF9C66DF0D1F7C09%40AM5PR83MB0178.EURPRD83.prod.outlook.com
I feel like this is the type of change where there's not much discussion to be had. And the only way to resolve it is to use some voting to gauge community opinion. So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or +1 to indicate support against/for the change. I'll start: +1 Attached is a patch that removes -Wdeclaration-after-statement in the codebase. This is mainly to be able to add it to the commitfest, to hopefully get a decent amount of responses.
Attachment
Jelte Fennema-Nio <postgres@jeltef.nl> writes: > I feel like this is the type of change where there's not much > discussion to be had. And the only way to resolve it is to use some > voting to gauge community opinion. > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. > I'll start: +1 [ shrug... ] -1 here. regards, tom lane
> On Jan 29, 2024, at 7:03 AM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. -1 for me. -Infinity for refactoring the entire codebase and backpatching. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, 29 Jan 2024 at 10:31, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> On Jan 29, 2024, at 7:03 AM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or
> +1 to indicate support against/for the change.
-1 for me.
-Infinity for refactoring the entire codebase and backpatching.
I don't think anybody is proposing re-working the existing codebase. I understand this to be only about allowing new code to use the newer style. Personally, I like, as much as possible, to use initializations to const variables and avoid assignment operators, so I much prefer to declare at first (and usually only) assignment.
But not voting because the amount of code I’ve actually contributed is minuscule.
On Mon, Jan 29, 2024 at 10:23:38AM -0500, Tom Lane wrote: > Jelte Fennema-Nio <postgres@jeltef.nl> writes: >> I feel like this is the type of change where there's not much >> discussion to be had. And the only way to resolve it is to use some >> voting to gauge community opinion. > >> So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or >> +1 to indicate support against/for the change. > >> I'll start: +1 > > [ shrug... ] -1 here. -1 here, too. I don't think one style is enormously better than the other, but I do like having the declarations in a predictable location. I actually find the alternative less readable, but that could just be because I spend so much time looking at Postgres code. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
> On Jan 29, 2024, at 7:35 AM, Isaac Morland <isaac.morland@gmail.com> wrote: > > On Mon, 29 Jan 2024 at 10:31, Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > -Infinity for refactoring the entire codebase and backpatching. > > I don't think anybody is proposing re-working the existing codebase. I understand this to be only about allowing new codeto use the newer style. Personally, I like, as much as possible, to use initializations to const variables and avoidassignment operators, so I much prefer to declare at first (and usually only) assignment. I was responding to Jelte's paragraph upthread: > On Dec 27, 2023, at 9:59 AM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > But if the only reason not to remove the > warning is that then there would be two styles of declaration in the > codebase, then I'm happy to create another refactoring script that > moves declarations down to their first usage. (Which could then be run > on all backbranches to make sure there is no backpatching pain) I find that kind of gratuitous code churn highly unpleasant. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Em seg., 29 de jan. de 2024 às 12:03, Jelte Fennema-Nio <postgres@jeltef.nl> escreveu:
I feel like this is the type of change where there's not much
discussion to be had. And the only way to resolve it is to use some
voting to gauge community opinion.
So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or
+1 to indicate support against/for the change.
I'll start: +1
Attached is a patch that removes -Wdeclaration-after-statement in the
codebase.
-1
C89 IMO is the best for C readability.
None surprise, once you read the block declaration,
You know all about the vars in the function.
Best regards,
Ranier Vilela
On Mon, Jan 29, 2024 at 10:03 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > I feel like this is the type of change where there's not much > discussion to be had. And the only way to resolve it is to use some > voting to gauge community opinion. > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. > > I'll start: +1 -1. I occasionally run into situations where I'm like "ah, it would be nicer to declare this in the middle of the block". But for every 1 time that happens, there are probably 10 times where it's helpful to me to be able to look at the top of the block and see all of the variable declarations in one place. Plus, a lot of times, this urge to declare mid-block is a sign that I've made that block too big and complex and I need to refactor and simplify. The fact that all of our code uses a consistent style is awfully nice, too. The main argument I see for changing anything is that we do a lot of things on this project that many people consider old-fashioned, and it may discourage some younger developers from getting involved in the project. I doubt that this is anywhere close to the biggest problem we have in that area, but if we do end up changing it I'll console myself with the thought that maybe we're usefully modernizing something. Personally, though, I prefer the status quo, where the correct location of a declaration for a particular identifier is largely an objective question rather than a subjective question. We are not in need of more bikeshedding targets. -- Robert Haas EDB: http://www.enterprisedb.com
On Mon, 29 Jan 2024 at 10:42, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> I don't think anybody is proposing re-working the existing codebase. I understand this to be only about allowing new code to use the newer style. Personally, I like, as much as possible, to use initializations to const variables and avoid assignment operators, so I much prefer to declare at first (and usually only) assignment.
I was responding to Jelte's paragraph upthread:
> On Dec 27, 2023, at 9:59 AM, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> But if the only reason not to remove the
> warning is that then there would be two styles of declaration in the
> codebase, then I'm happy to create another refactoring script that
> moves declarations down to their first usage. (Which could then be run
> on all backbranches to make sure there is no backpatching pain)
I find that kind of gratuitous code churn highly unpleasant.
I stand corrected, and agree completely. It’s hard to imagine a change of such a global nature that would be a big enough improvement that it would be a good idea to apply to existing code. Personally I’m fine with code of different vintages using different styles, as long as it’s understood why the difference exists — in this case because tons of code has already been written and isn’t going to be re-styled except possibly as part of other changes.
On 29/01/2024 19:07, Robert Haas wrote: > On Mon, Jan 29, 2024 at 10:03 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: >> I feel like this is the type of change where there's not much >> discussion to be had. And the only way to resolve it is to use some >> voting to gauge community opinion. >> >> So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or >> +1 to indicate support against/for the change. >> >> I'll start: +1 > > -1. I occasionally run into situations where I'm like "ah, it would be > nicer to declare this in the middle of the block". But for every 1 > time that happens, there are probably 10 times where it's helpful to > me to be able to look at the top of the block and see all of the > variable declarations in one place. Plus, a lot of times, this urge to > declare mid-block is a sign that I've made that block too big and > complex and I need to refactor and simplify. -0.5 from me, for exactly those reasons Robert said. I wouldn't mind removing the compiler flag as long as we mostly keep the current style of declarations at top, with exceptions when it really makes sense. But in practice it would open the floodgates and make things worse overall. You can also add curly braces to introduce a block like this: do_stuff(); { int i = 123; do_more_stuff(i); ... } I know many people dislike that too, though. I think it's usually better than declaring a variable in the middle of a block, because it also makes you think how long the variable needs to be in scope. > The main argument I see for changing anything is that we do a lot of > things on this project that many people consider old-fashioned, and it > may discourage some younger developers from getting involved in the > project. I doubt that this is anywhere close to the biggest problem we > have in that area, but if we do end up changing it I'll console myself > with the thought that maybe we're usefully modernizing something. People writing extensions or hacking on their own forks of PostgreSQL are free to do whatever they like. And I don't mind reviewing patches that don't follow the usual guidelines to the letter, stylistic things like this are easy to fix before committing. I don't feel like we're forcing these rules upon others. -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Jan 29, 2024, at 12:03 PM, Jelte Fennema-Nio wrote:
I feel like this is the type of change where there's not muchdiscussion to be had. And the only way to resolve it is to use somevoting to gauge community opinion.So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or+1 to indicate support against/for the change.I'll start: +1
-1 here. Unless you have a huge amount of variables, (1) is an issue. The most
annoying argument against the current style (declarations at the top of the
function) is (2). However, people usually say it is a code smell to have a big
chunk of code into a single function and that you need to split the code into
multiple functions. Having said that, it is easier to check if the variable V
is used because the number of lines you need to read is small. And it imposes
similar effort to inspect the code than your argument (3), (4), and (5).
One argument against it is if you use the "declare on first use" style, you
will spend more time inspecting the code if you are refactoring it. You
identify the code block you want to move and than starts the saga. Check every
single variable used by this code block and search for its declaration. It is
such a time consuming task. However, if you are using the "declare at the top"
style, it is a matter of checking at the top.
Keep both styles can be rather confusing (in particular for newbies). And as
Nathan said I don't see huge benefits moving from one style to the other.
Hi, On 2023-12-27 12:48:40 +0100, Jelte Fennema-Nio wrote: > Postgres currently requires all variables to be declared at the top of > the function, because it specifies -Wdeclaration-after-statement. One > of the reasons that we had this warning was because C89 required this > style of declaration. Requiring it everywhere made backporting easier, > since some of our older supported PG versions needed to compile on > C89. Now that we have dropped support for PG11 that reason goes away, > since now all supported Postgres versions require C99. So, I think > it's worth reconsidering if we want this warning to be enabled or not. +1 for allowing declarations to be intermixed with code, -infinity for changing existing code to do so. Greetings, Andres Freund
On Mon, Jan 29, 2024 at 1:38 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > -0.5 from me, for exactly those reasons Robert said. I wouldn't mind > removing the compiler flag as long as we mostly keep the current style > of declarations at top, with exceptions when it really makes sense. But > in practice it would open the floodgates and make things worse overall. Yeah, this, for sure. If it were done judiciously I wouldn't care, but in practice it wouldn't be. Different people would do wildly different things, ranging from never putting anything mid-block at all, at one extreme, to rearranging the whole flow of the function to allow for more mid-block declarations, at the other. TBH, I wish we could get more consistent about our coding style overall, and clean up some of our historical baggage. We have such beautiful code in so many places, and such ugly code in others. I still can't get over how ugly xlog.c is in particular. Multiple people have attempted to split that file up, or clean it up in other ways, but it's still a soup of unclear global variables and identifier names pulled out of a hat. And it still baffles me why we allow everyone to pick their own system for capitalizing identifiers out of a hat, without even insisting on consistency from one end of the same identifier to the other. > You can also add curly braces to introduce a block like this: > > do_stuff(); > { > int i = 123; > > do_more_stuff(i); > ... > } > > I know many people dislike that too, though. I think it's usually better > than declaring a variable in the middle of a block, because it also > makes you think how long the variable needs to be in scope. I think this style can be appropriate for assertions or debugging code, where you only need the variable if some compiler symbol is defined, and you isolate it to the same block where it's used. I don't tend to like this style for other cases. It looks like you were too lazy to go back to the top of the function and just add the declaration there. I've also found that when I'm uncomfortable moving the variable to the beginning of the block because it doesn't seem to fit with the other stuff declared there, it's usually a sign that I'm going to end up making the block conditional or turning it into a separate function -- and of course if I do either of those things, then suddenly I have a natural scope for my declarations. -- Robert Haas EDB: http://www.enterprisedb.com
On 2024-01-29 Mo 14:58, Andres Freund wrote: > Hi, > > On 2023-12-27 12:48:40 +0100, Jelte Fennema-Nio wrote: >> Postgres currently requires all variables to be declared at the top of >> the function, because it specifies -Wdeclaration-after-statement. One >> of the reasons that we had this warning was because C89 required this >> style of declaration. Requiring it everywhere made backporting easier, >> since some of our older supported PG versions needed to compile on >> C89. Now that we have dropped support for PG11 that reason goes away, >> since now all supported Postgres versions require C99. So, I think >> it's worth reconsidering if we want this warning to be enabled or not. > +1 for allowing declarations to be intermixed with code, I'm about +0.5. Many Java, C++, Perl, and indeed C programmers might find it surprising that we're having this debate. On the more modern language front the same goes for Go and Rust. It seems clear that the language trend is mostly in this direction. But it's not something worth having a long and contentious debate over. We have plenty of better candidates for that :-) > -infinity for > changing existing code to do so. ditto. On that at least I think there's close to unanimous agreement. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Hi, On 2024-01-29 15:01:06 -0500, Robert Haas wrote: > And it still baffles me why we allow everyone to pick their own system for > capitalizing identifiers out of a hat, without even insisting on consistency > from one end of the same identifier to the other. Yes. Please. I hate some capitalization/underscore styles, but I hate spending time feeling out which capitalization style I should use so much more. Let's at least define some minimal naming guidelines for new code. Personally I like under_score_style for functions and variables and CamelCase for types. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2024-01-29 15:01:06 -0500, Robert Haas wrote: >> And it still baffles me why we allow everyone to pick their own system for >> capitalizing identifiers out of a hat, without even insisting on consistency >> from one end of the same identifier to the other. > Yes. Please. I hate some capitalization/underscore styles, but I hate spending > time feeling out which capitalization style I should use so much more. Let's > at least define some minimal naming guidelines for new code. I'm for this for entirely-new code, but I think when adding code in existing modules we're better off with the rule of "make it match nearby code". I admit it might be hard to draw a clear line between the two cases, plus there might be local inconsistency already. But let's try to avoid making local style inconsistencies worse. regards, tom lane
Hi, On January 29, 2024 2:09:23 PM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote: >Andres Freund <andres@anarazel.de> writes: >> On 2024-01-29 15:01:06 -0500, Robert Haas wrote: >>> And it still baffles me why we allow everyone to pick their own system for >>> capitalizing identifiers out of a hat, without even insisting on consistency >>> from one end of the same identifier to the other. > >> Yes. Please. I hate some capitalization/underscore styles, but I hate spending >> time feeling out which capitalization style I should use so much more. Let's >> at least define some minimal naming guidelines for new code. > >I'm for this for entirely-new code, but I think when adding code in >existing modules we're better off with the rule of "make it match >nearby code". I admit it might be hard to draw a clear line between >the two cases, plus there might be local inconsistency already. >But let's try to avoid making local style inconsistencies worse. Yeah, completely agreed. I think using it as a tie breaker when extending already inconsistent code, of which we have plenty,is the extent of the role it should have when extending existing code. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On 29.01.24 16:03, Jelte Fennema-Nio wrote: > I feel like this is the type of change where there's not much > discussion to be had. And the only way to resolve it is to use some > voting to gauge community opinion. > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. > > I'll start: +1 > > Attached is a patch that removes -Wdeclaration-after-statement in the > codebase. This is mainly to be able to add it to the commitfest, to > hopefully get a decent amount of responses. -1, mostly for the various reasons explained by others.
On Mon, Jan 29, 2024 at 11:04 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote: > > I feel like this is the type of change where there's not much > discussion to be had. And the only way to resolve it is to use some > voting to gauge community opinion. > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. > > I'll start: +1 > > Attached is a patch that removes -Wdeclaration-after-statement in the > codebase. This is mainly to be able to add it to the commitfest, to > hopefully get a decent amount of responses. I am new to c. gcc has many options (https://man7.org/linux/man-pages/man1/gcc.1.html) IIUC, postgres have some required CFLAGS, While doing the test (http://cfbot.cputube.org/next.html), we also have some CFLAGS I am not sure they are the same. if we can list these CFLAGS information in the https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F would be helpful. But not voting because I am not sure of the implication.
On Mon, Jan 29, 2024 at 04:03:44PM +0100, Jelte Fennema-Nio wrote: > I feel like this is the type of change where there's not much > discussion to be had. And the only way to resolve it is to use some > voting to gauge community opinion. > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > +1 to indicate support against/for the change. I'm +1 for the change, for these reasons: - Fewer back-patch merge conflicts. The decls section of long functions is a classic conflict point. - A mid-func decl demonstrates that its var is unused in the first half of the func. - We write Perl in the mixed decls style, without problems. For me personally, the "inconsistency" concern is negligible. We allowed "for (int i = 0", and that inconsistency has been invisible to me.
On Wed, Feb 7, 2024 at 7:55 PM Noah Misch <noah@leadboat.com> wrote: > > So my suggestion is for people to respond with -1, -0.5, +-0, +0.5, or > > +1 to indicate support against/for the change. > > I'm +1 for the change, for these reasons: > > - Fewer back-patch merge conflicts. The decls section of long functions is a > classic conflict point. > - A mid-func decl demonstrates that its var is unused in the first half of the > func. > - We write Perl in the mixed decls style, without problems. > > For me personally, the "inconsistency" concern is negligible. We allowed "for > (int i = 0", and that inconsistency has been invisible to me. This thread was interesting as an opinion poll, but it seems clear that the consensus is still against the proposed change, so I've marked the CommitFest entry rejected. -- Robert Haas EDB: http://www.enterprisedb.com