Re: [HACKERS] Running pgindent
* Bruce Momjian (br...@momjian.us) wrote: On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: I spent quite a lot of time trying to make the tool behave the same as the old script. Yes, and I believe we tested running the Perl version to make sure it was the same, so the changes we are seeing are just normal (unfortunate) adjustments by pgindent. Just to wrap this up- I wanted to say thanks to both you (Bruce) and to Andrew for making pgindent work and the documentation / instructions easy to follow. In the past, pgindent has always seemed to be a black art, where it was difficult to get consistent results across architectures due to different typedef lists, etc. Now, at least for me, it 'just worked'. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Running pgindent
Stephen Frost escribió: Just to wrap this up- I wanted to say thanks to both you (Bruce) and to Andrew for making pgindent work and the documentation / instructions easy to follow. In the past, pgindent has always seemed to be a black art, where it was difficult to get consistent results across architectures due to different typedef lists, etc. Now, at least for me, it 'just worked'. Yes, agreed. I tend to add a pgindent step to my patch submissions (even though my editor does a pretty good job), and this has been made possible by this work. Thanks guys. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
* Bruce Momjian (br...@momjian.us) wrote: OK. Done. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Running pgindent
On 05/29/2013 11:41 PM, Bruce Momjian wrote: On Wed, May 29, 2013 at 10:08:10PM -0400, Stephen Frost wrote: * Bruce Momjian (br...@momjian.us) wrote: Wow, uh, yeah, I guess we could do that. I will await more feedback. Please don't. I'm already rather concerned by this one. It looks like there's a rule to pull a line in to meet the max-column requirement even when that makes things line up 'funny', eg: I did a comparison of the parameters passed to BSD indent, and Andrew did accurately transfer all the flags, so I am a little confused why there is such a difference, as BSD indent has not changed. I spent quite a lot of time trying to make the tool behave the same as the old script. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: On 05/29/2013 11:41 PM, Bruce Momjian wrote: On Wed, May 29, 2013 at 10:08:10PM -0400, Stephen Frost wrote: * Bruce Momjian (br...@momjian.us) wrote: Wow, uh, yeah, I guess we could do that. I will await more feedback. Please don't. I'm already rather concerned by this one. It looks like there's a rule to pull a line in to meet the max-column requirement even when that makes things line up 'funny', eg: I did a comparison of the parameters passed to BSD indent, and Andrew did accurately transfer all the flags, so I am a little confused why there is such a difference, as BSD indent has not changed. I spent quite a lot of time trying to make the tool behave the same as the old script. Yes, and I believe we tested running the Perl version to make sure it was the same, so the changes we are seeing are just normal (unfortunate) adjustments by pgindent. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
* Bruce Momjian (br...@momjian.us) wrote: On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: I spent quite a lot of time trying to make the tool behave the same as the old script. Yes, and I believe we tested running the Perl version to make sure it was the same, so the changes we are seeing are just normal (unfortunate) adjustments by pgindent. Fair enough, thanks for the efforts and for looking into it, apologies for my complaining. If no one objects, I'm going to review the conditional statements and similar lines which could be split out on to multiple lines instead of being squeezed closer to the left edge and not lined up properly. Then I'll try and get pgindent running locally. If I can get that happening and it doesn't want to further change the modifications that I make, then I'll commit those cleanups. This will only be for changes made during this latest pgindent run and only against master- I don't want to create any more code churn beyond what's already being impacted. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Running pgindent
On Fri, May 31, 2013 at 04:57:20PM -0400, Stephen Frost wrote: * Bruce Momjian (br...@momjian.us) wrote: On Fri, May 31, 2013 at 03:51:30PM -0400, Andrew Dunstan wrote: I spent quite a lot of time trying to make the tool behave the same as the old script. Yes, and I believe we tested running the Perl version to make sure it was the same, so the changes we are seeing are just normal (unfortunate) adjustments by pgindent. Fair enough, thanks for the efforts and for looking into it, apologies for my complaining. If no one objects, I'm going to review the conditional statements and similar lines which could be split out on to multiple lines instead of being squeezed closer to the left edge and not lined up properly. Then I'll try and get pgindent running locally. If I can get that happening and it doesn't want to further change the modifications that I make, then I'll commit those cleanups. This will only be for changes made during this latest pgindent run and only against master- I don't want to create any more code churn beyond what's already being impacted. OK. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Tue, May 28, 2013 at 09:56:03AM -0400, Bruce Momjian wrote: On Tue, May 28, 2013 at 09:49:32AM -0400, Magnus Hagander wrote: On Tue, May 28, 2013 at 9:48 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian br...@momjian.us wrote: On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? wfm. +1. OK, consider it scheduled, 2013-05-29, 1400 ET, 1800 GMT. Done. This was the first run of the Perl-based pgindent script. There was a lot of code churn in this run as the paragraphs are slightly wider. Also, I saw some outdenting of long lines, rather than allowing them to go past 80 characters, but it seemed minimal. If others see problems, we can adjust the script and run it again. All tests passed. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
Bruce Momjian escribió: Done. This was the first run of the Perl-based pgindent script. There was a lot of code churn in this run as the paragraphs are slightly wider. Also, I saw some outdenting of long lines, rather than allowing them to go past 80 characters, but it seemed minimal. If others see problems, we can adjust the script and run it again. It seems a bit troubling to me that the underlying indent binary seems to have behaved slightly different. I think this will cause problems down the road when we try to backpatch stuff that has been indented differently on the older branches. I wonder if it would be best to inflict pgindent with the new script upon the old active branches as well, to avoid this problem. (It might cause other problems, of course.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Wed, May 29, 2013 at 05:56:32PM -0400, Alvaro Herrera wrote: Bruce Momjian escribió: Done. This was the first run of the Perl-based pgindent script. There was a lot of code churn in this run as the paragraphs are slightly wider. Also, I saw some outdenting of long lines, rather than allowing them to go past 80 characters, but it seemed minimal. If others see problems, we can adjust the script and run it again. It seems a bit troubling to me that the underlying indent binary seems to have behaved slightly different. I think this will cause problems down the road when we try to backpatch stuff that has been indented differently on the older branches. Well, we have to figure out why it is behaving differently. If I had to guess it would be that the parameters to BSD indent were somehow different, though I thought Andrew copied them directly. I wonder if it would be best to inflict pgindent with the new script upon the old active branches as well, to avoid this problem. (It might cause other problems, of course.) Wow, uh, yeah, I guess we could do that. I will await more feedback. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
* Bruce Momjian (br...@momjian.us) wrote:
Wow, uh, yeah, I guess we could do that. I will await more feedback.
Please don't. I'm already rather concerned by this one. It looks like
there's a rule to pull a line in to meet the max-column requirement even
when that makes things line up 'funny', eg:
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
*** hstore_to_json_loose(PG_FUNCTION_ARGS)
*** 1300,1306
* digit as numeric - could be a zip code or similar
*/
if (src-len 0
! !(src-data[0] == '0' isdigit((unsigned char) src-data[1]))
strspn(src-data, +-0123456789Ee.) == src-len)
{
/*
--- 1300,1306
* digit as numeric - could be a zip code or similar
*/
if (src-len 0
! !(src-data[0] == '0' isdigit((unsigned char) src-data[1]))
strspn(src-data, +-0123456789Ee.) == src-len)
{
/*
For this case, perhaps we should just split it on to multiple lines, but
when there's not much on the line beyond a long string, I thought our
policy was to allow the line to go beyond the column limit? That's
certainly how section 49.1 reads to me; any chance we can get indent to
understand that?
We also claim that our indent runs won't screw around with comment
blocks, but it looks to pretty clearly be doing exactly that..
Another interesting case is this:
*** start_postmaster(ClusterInfo *cluster, b
*** 229,235
* it might supply a reason for the failure.
*/
pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
! /* pass both file names if they differ */
(strcmp(SERVER_LOG_FILE,
SERVER_START_LOG_FILE) != 0) ?
SERVER_LOG_FILE : NULL,
--- 229,235
* it might supply a reason for the failure.
*/
pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
! /* pass both file names if they differ */
(strcmp(SERVER_LOG_FILE,
SERVER_START_LOG_FILE) != 0) ?
SERVER_LOG_FILE : NULL,
It seems that a comment inside of parameters passed to a function isn't
indented to the same depth of the other arguments by this run, but I'm
guessing it was by prior versions.
Also doesn't seem to indent operations the same way that function
parameters are indented, eg:
*** pgrowlocks(PG_FUNCTION_ARGS)
*** 166,172
values[Atnum_ismulti] = pstrdup(true);
allow_old = !(infomask HEAP_LOCK_MASK)
!(infomask HEAP_XMAX_LOCK_ONLY);
nmembers = GetMultiXactIdMembers(xmax, members, allow_old);
if (nmembers == -1)
{
--- 166,172
values[Atnum_ismulti] = pstrdup(true);
allow_old = !(infomask HEAP_LOCK_MASK)
! (infomask HEAP_XMAX_LOCK_ONLY);
nmembers = GetMultiXactIdMembers(xmax, members, allow_old);
if (nmembers == -1)
{
Which seems to also be 'wrong' to my eyes.
In general, there are a lot of improvements being made, but I don't like
what appear to be regressions. :)
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Running pgindent
On Wed, May 29, 2013 at 10:08:10PM -0400, Stephen Frost wrote:
* Bruce Momjian (br...@momjian.us) wrote:
Wow, uh, yeah, I guess we could do that. I will await more feedback.
Please don't. I'm already rather concerned by this one. It looks like
there's a rule to pull a line in to meet the max-column requirement even
when that makes things line up 'funny', eg:
I did a comparison of the parameters passed to BSD indent, and Andrew
did accurately transfer all the flags, so I am a little confused why
there is such a difference, as BSD indent has not changed.
*** a/contrib/hstore/hstore_io.c
--- b/contrib/hstore/hstore_io.c
*** hstore_to_json_loose(PG_FUNCTION_ARGS)
*** 1300,1306
* digit as numeric - could be a zip code or similar
*/
if (src-len 0
! !(src-data[0] == '0' isdigit((unsigned char)
src-data[1]))
strspn(src-data, +-0123456789Ee.) == src-len)
{
/*
--- 1300,1306
* digit as numeric - could be a zip code or similar
*/
if (src-len 0
! !(src-data[0] == '0' isdigit((unsigned char) src-data[1]))
strspn(src-data, +-0123456789Ee.) == src-len)
{
/*
For this case, perhaps we should just split it on to multiple lines, but
when there's not much on the line beyond a long string, I thought our
policy was to allow the line to go beyond the column limit? That's
certainly how section 49.1 reads to me; any chance we can get indent to
understand that?
I thought we used to do that too. However, I don't see this line in the
9.2 hstore source, so I assume it is new and that pgindent is doing what
it used to do.
We also claim that our indent runs won't screw around with comment
blocks, but it looks to pretty clearly be doing exactly that..
We only promise that if you use /* markers.
Another interesting case is this:
*** start_postmaster(ClusterInfo *cluster, b
*** 229,235
* it might supply a reason for the failure.
*/
pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
! /* pass both file names if they differ */
(strcmp(SERVER_LOG_FILE,
SERVER_START_LOG_FILE) != 0) ?
SERVER_LOG_FILE : NULL,
--- 229,235
* it might supply a reason for the failure.
*/
pg_ctl_return = exec_prog(SERVER_START_LOG_FILE,
! /* pass both file names if they differ */
(strcmp(SERVER_LOG_FILE,
SERVER_START_LOG_FILE) != 0) ?
SERVER_LOG_FILE : NULL,
It seems that a comment inside of parameters passed to a function isn't
indented to the same depth of the other arguments by this run, but I'm
guessing it was by prior versions.
Uh, this comment was definitely in 9.2, but was added as a backpatch by
Tom in September 2012, after pg_upgrade was run on the 9.2 code.
Also doesn't seem to indent operations the same way that function
parameters are indented, eg:
*** pgrowlocks(PG_FUNCTION_ARGS)
*** 166,172
values[Atnum_ismulti] = pstrdup(true);
allow_old = !(infomask HEAP_LOCK_MASK)
!(infomask HEAP_XMAX_LOCK_ONLY);
nmembers = GetMultiXactIdMembers(xmax, members, allow_old);
if (nmembers == -1)
{
--- 166,172
values[Atnum_ismulti] = pstrdup(true);
allow_old = !(infomask HEAP_LOCK_MASK)
! (infomask HEAP_XMAX_LOCK_ONLY);
nmembers = GetMultiXactIdMembers(xmax, members, allow_old);
if (nmembers == -1)
{
Which seems to also be 'wrong' to my eyes.
Yes, it certainly does, but this line also does not exist in 9.2 source
tree.
In general, there are a lot of improvements being made, but I don't like
what appear to be regressions. :)
I agree. Can someone fine a case that was run through 9.2 pgindent that
is worse now? Every case I could find either was the same in 9.2 or was
changed after the 9.2 pgindent run.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Tue, May 28, 2013 at 9:48 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian br...@momjian.us wrote: On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? wfm. +1. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Tue, May 28, 2013 at 09:49:32AM -0400, Magnus Hagander wrote: On Tue, May 28, 2013 at 9:48 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian br...@momjian.us wrote: On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? wfm. +1. OK, consider it scheduled, 2013-05-29, 1400 ET, 1800 GMT. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Tue, May 28, 2013 at 9:40 AM, Bruce Momjian br...@momjian.us wrote: On Wed, May 22, 2013 at 01:52:28PM -0400, Bruce Momjian wrote: Do we want to run pgindent soon? OK, should I run it this week? Wednesday, 1800 GMT? wfm. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Running pgindent
Do we want to run pgindent soon? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Running pgindent
On Wed, May 22, 2013 at 1:52 PM, Bruce Momjian br...@momjian.us wrote: Do we want to run pgindent soon? +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
