Обсуждение: 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.

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, 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

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 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)

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

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