Обсуждение: [HACKERS] Re: [COMMITTERS] pgsql: Preventive maintenance in advance ofpgindent run.

Поиск
Список
Период
Сортировка

[HACKERS] Re: [COMMITTERS] pgsql: Preventive maintenance in advance ofpgindent run.

От
Alvaro Herrera
Дата:
Tom Lane wrote:

> src/bin/pg_basebackup/pg_basebackup.c          | 24 +++++++++---------
> src/bin/pg_waldump/pg_waldump.c                | 18 ++++++-------

There are some changes here that should be reverted; for instance:

-   printf(_("  -c, --checkpoint=fast|spread\n"
-            "                         set fast or spread checkpointing\n"));
+   printf(_("  -c, --checkpoint=fast|spread\n"));
+   printf(_("                         set fast or spread checkpointing\n"));

From the translator's point of view the patched version doesn't make
sense because they are two separate strings.  In the original, it's a
single translatable string.  Particularly in pg_waldump's -p, where a
phrase is now cut in the middle.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> There are some changes here that should be reverted; for instance:

> -   printf(_("  -c, --checkpoint=fast|spread\n"
> -            "                         set fast or spread checkpointing\n"));
> +   printf(_("  -c, --checkpoint=fast|spread\n"));
> +   printf(_("                         set fast or spread checkpointing\n"));

> From the translator's point of view the patched version doesn't make
> sense because they are two separate strings.  In the original, it's a
> single translatable string.  Particularly in pg_waldump's -p, where a
> phrase is now cut in the middle.

What I was concerned about was that pgindent will reindent the second
line so that it's impossible to tell whether the spacing is correct.
That might not matter to translators but it will be a problem for
source-level maintenance.

Maybe we should rethink the whole idea of breaking these entries across
lines, and just accept that the commentary doesn't line up with other
lines:
  printf(_("  -c, --checkpoint=fast|spread  set fast or spread checkpointing\n"));

Thoughts?
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> ... Particularly in pg_waldump's -p, where a
>> phrase is now cut in the middle.

BTW, I would say that the problem with -p is that somebody failed to
understand the difference between --help and a man page.  That entry
should be
 -p, --path=PATH        directory in which to find log segment files

full stop.  I'm not sure that the other multi-line entries in
pg_waldump's --help have an excuse to live, either.
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
I wrote:
> BTW, I would say that the problem with -p is that somebody failed to
> understand the difference between --help and a man page.

Concretely, how about the attached?  I don't think this looks any
worse than the current layout.

            regards, tom lane

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 866f88a..f2492e4 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -331,22 +331,17 @@ usage(void)
     printf(_("\nOptions controlling the output:\n"));
     printf(_("  -D, --pgdata=DIRECTORY receive base backup into directory\n"));
     printf(_("  -F, --format=p|t       output format (plain (default), tar)\n"));
-    printf(_("  -r, --max-rate=RATE    maximum transfer rate to transfer data directory\n"));
-    printf(_("                         (in kB/s, or use suffix \"k\" or \"M\")\n"));
-    printf(_("  -R, --write-recovery-conf\n"));
-    printf(_("                         write recovery.conf for replication\n"));
+    printf(_("  -r, --max-rate=RATE    maximum transfer rate (in kB/s)\n"));
+    printf(_("  -R, --write-recovery-conf  write recovery.conf for replication\n"));
     printf(_("  -S, --slot=SLOTNAME    replication slot to use\n"));
     printf(_("      --no-slot          prevent creation of temporary replication slot\n"));
-    printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR\n"));
-    printf(_("                         relocate tablespace in OLDDIR to NEWDIR\n"));
-    printf(_("  -X, --wal-method=none|fetch|stream\n"));
-    printf(_("                         include required WAL files with specified method\n"));
+    printf(_("  -T, --tablespace-mapping=OLDDIR=NEWDIR  relocate tablespace\n"));
+    printf(_("  -X, --wal-method=none|fetch|stream  set method for including WAL files\n"));
     printf(_("      --waldir=WALDIR    location for the write-ahead log directory\n"));
     printf(_("  -z, --gzip             compress tar output\n"));
     printf(_("  -Z, --compress=0-9     compress tar output with given compression level\n"));
     printf(_("\nGeneral options:\n"));
-    printf(_("  -c, --checkpoint=fast|spread\n"));
-    printf(_("                         set fast or spread checkpointing\n"));
+    printf(_("  -c, --checkpoint=fast|spread  set fast or spread checkpointing\n"));
     printf(_("  -l, --label=LABEL      set backup label\n"));
     printf(_("  -n, --no-clean         do not clean up after errors\n"));
     printf(_("  -N, --no-sync          do not wait for changes to be written safely to disk\n"));
@@ -358,8 +353,7 @@ usage(void)
     printf(_("  -d, --dbname=CONNSTR   connection string\n"));
     printf(_("  -h, --host=HOSTNAME    database server host or socket directory\n"));
     printf(_("  -p, --port=PORT        database server port number\n"));
-    printf(_("  -s, --status-interval=INTERVAL\n"));
-    printf(_("                         time between status packets sent to server (in seconds)\n"));
+    printf(_("  -s, --status-interval=SECONDS  set time between status packets\n"));
     printf(_("  -U, --username=NAME    connect as specified database user\n"));
     printf(_("  -w, --no-password      never prompt for password\n"));
     printf(_("  -W, --password         force password prompt (should happen automatically)\n"));
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 56843a5..e56d7e2 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -689,18 +689,14 @@ usage(void)
     printf(_("  -e, --end=RECPTR       stop reading at WAL location RECPTR\n"));
     printf(_("  -f, --follow           keep retrying after reaching end of WAL\n"));
     printf(_("  -n, --limit=N          number of records to display\n"));
-    printf(_("  -p, --path=PATH        directory in which to find log segment files or a\n"));
-    printf(_("                         directory with a ./pg_wal that contains such files\n"));
-    printf(_("                         (default: current directory, ./pg_wal, PGDATA/pg_wal)\n"));
+    printf(_("  -p, --path=PATH        directory in which to find log segment files\n"));
     printf(_("  -r, --rmgr=RMGR        only show records generated by resource manager RMGR\n"));
     printf(_("                         use --rmgr=list to list valid resource manager names\n"));
     printf(_("  -s, --start=RECPTR     start reading at WAL location RECPTR\n"));
     printf(_("  -t, --timeline=TLI     timeline from which to read log records\n"));
-    printf(_("                         (default: 1 or the value used in STARTSEG)\n"));
     printf(_("  -V, --version          output version information, then exit\n"));
     printf(_("  -x, --xid=XID          only show records with TransactionId XID\n"));
-    printf(_("  -z, --stats[=record]   show statistics instead of records\n"));
-    printf(_("                         (optionally, show per-record statistics)\n"));
+    printf(_("  -z, --stats[=record]   show statistics or per-record statistics, not records\n"));
     printf(_("  -?, --help             show this help, then exit\n"));
 }


-- 
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] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Peter Eisentraut
Дата:
On 5/17/17 11:37, Tom Lane wrote:
> I wrote:
>> BTW, I would say that the problem with -p is that somebody failed to
>> understand the difference between --help and a man page.
> 
> Concretely, how about the attached?  I don't think this looks any
> worse than the current layout.

The previous setup has been in place for years and has never been a
problem.  The alternatives are all quite a bit worse.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Peter Eisentraut
Дата:
On 5/17/17 10:14, Tom Lane wrote:
> What I was concerned about was that pgindent will reindent the second
> line so that it's impossible to tell whether the spacing is correct.

pgindent moving string continuations to the left is a completely
terrible behavior anyway and we should look into changing that.  Just
look at the mess it makes out of SELECT queries in pg_dump.c.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 5/17/17 11:37, Tom Lane wrote:
>> Concretely, how about the attached?  I don't think this looks any
>> worse than the current layout.

> The previous setup has been in place for years and has never been a
> problem.  The alternatives are all quite a bit worse.

No, the previous setup hasn't been "in place for years".  These programs
were only NLS-ified last fall.  Before that the code looked like, eg,
printf("  -z, --stats[=record]   show statistics instead of records\n");printf("                         (optionally,
showper-record statistics)\n"); 

so that there weren't string continuations for pgindent to fool with.

I'm not really convinced that having usage() print two- or three-line
switch descriptions is "quite a bit better" than what I suggested.
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> On 5/17/17 10:14, Tom Lane wrote:
>> What I was concerned about was that pgindent will reindent the second
>> line so that it's impossible to tell whether the spacing is correct.

> pgindent moving string continuations to the left is a completely
> terrible behavior anyway and we should look into changing that.  Just
> look at the mess it makes out of SELECT queries in pg_dump.c.

I'd be on board with that, perhaps, but does anyone here have enough
insight into bsd_indent to fix that without breaking other stuff?
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Alvaro Herrera
Дата:
Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > On 5/17/17 11:37, Tom Lane wrote:
> >> Concretely, how about the attached?  I don't think this looks any
> >> worse than the current layout.
> 
> > The previous setup has been in place for years and has never been a
> > problem.  The alternatives are all quite a bit worse.
> 
> No, the previous setup hasn't been "in place for years".  These programs
> were only NLS-ified last fall.

We use the same technique in other places such as pg_dump's help() too.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> No, the previous setup hasn't been "in place for years".  These programs
>> were only NLS-ified last fall.

> We use the same technique in other places such as pg_dump's help() too.

Meh.  Well, I reverted the changes in question while we discuss it.

Changing the pgindent rule for such cases sounds kind of promising,
but will anyone pursue it?

(In any case, we should probably go ahead with the scheduled pgindent
run, and then we can consider a new run with new rules later; that
will ease seeing exactly what changes a new rule would make.)
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Alvaro Herrera
Дата:
Tom Lane wrote:

> Changing the pgindent rule for such cases sounds kind of promising,
> but will anyone pursue it?

We have someone who has studied the BSD indent code and even sent us
patches to fix quite a few bugs, but we've largely ignored his efforts
so far.  I propose we take that indent as part of our repo, and patch it
to our preferences.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> We have someone who has studied the BSD indent code and even sent us
> patches to fix quite a few bugs, but we've largely ignored his efforts
> so far.  I propose we take that indent as part of our repo, and patch it
> to our preferences.

Messing with pgindent didn't seem all that high-priority while we were
in development mode, and it would also have been pretty painful to have
a pgindent that wanted to revisit settled decisions when you just wanted
it to fix new code.  Maybe now (ie, over the next few weeks) is a good
time to pursue it, before we start a new round of code development.

Not sure about actually incorporating it into our repo.  Doing so would
make it easier for people to use, for sure, and the license seems to be
regular 3-clause BSD, so that angle is OK.  But do we want to be carrying
around another 150K of source code?
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Bruce Momjian
Дата:
On Wed, May 17, 2017 at 01:06:26PM -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > Tom Lane wrote:
> >> No, the previous setup hasn't been "in place for years".  These programs
> >> were only NLS-ified last fall.
> 
> > We use the same technique in other places such as pg_dump's help() too.
> 
> Meh.  Well, I reverted the changes in question while we discuss it.

Are we ready for a pgindent run now?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Piotr Stefaniak
Дата:
On 2017-05-17 19:16, Alvaro Herrera wrote:
> Tom Lane wrote:
>
>> Changing the pgindent rule for such cases sounds kind of promising,
>> but will anyone pursue it?
>
> We have someone who has studied the BSD indent code and even sent us
> patches to fix quite a few bugs, but we've largely ignored his efforts
> so far.  I propose we take that indent as part of our repo, and patch it
> to our preferences.

That, I assume, would be me. Coincidentally, I'm about to push my fixes
upstream (FreeBSD). Before that happens, my changes can be obtained from
https://github.com/pstef/freebsd_indent and tested, if anyone wishes.




Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Bruce Momjian <bruce@momjian.us> writes:
> Are we ready for a pgindent run now?

Yes, might as well do it.  Some of these discussions might lead to
a re-run with a newer version of pgindent, but it would be good to
have a clean tree to start from.
        regards, tom lane



Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > We have someone who has studied the BSD indent code and even sent us
> > patches to fix quite a few bugs, but we've largely ignored his efforts
> > so far.  I propose we take that indent as part of our repo, and patch it
> > to our preferences.
> 
> Messing with pgindent didn't seem all that high-priority while we were
> in development mode, and it would also have been pretty painful to have
> a pgindent that wanted to revisit settled decisions when you just wanted
> it to fix new code.  Maybe now (ie, over the next few weeks) is a good
> time to pursue it, before we start a new round of code development.

Sounds reasonable.

> Not sure about actually incorporating it into our repo.  Doing so would
> make it easier for people to use, for sure, and the license seems to be
> regular 3-clause BSD, so that angle is OK.  But do we want to be carrying
> around another 150K of source code?

The alternatives are

1. rely on the dead code we've been using so far (the old BSD indent
patched with our Pg-specific tweaks), or

2. rely on someone else's upstream code -- in this case, FreeBSD's as
patched by Piotr.

Now that Piotr's is about to find a home, perhaps it's okay for us to
rely on that one.  I just didn't like the idea of running something from
Piotr's personal repo.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Stephen Frost
Дата:
Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Are we ready for a pgindent run now?
>
> Yes, might as well do it.  Some of these discussions might lead to
> a re-run with a newer version of pgindent, but it would be good to
> have a clean tree to start from.

+1.

Thanks!

Stephen

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-05-17 19:16, Alvaro Herrera wrote:
>> We have someone who has studied the BSD indent code and even sent us
>> patches to fix quite a few bugs, but we've largely ignored his efforts
>> so far.  I propose we take that indent as part of our repo, and patch it
>> to our preferences.

> That, I assume, would be me. Coincidentally, I'm about to push my fixes
> upstream (FreeBSD). Before that happens, my changes can be obtained from
> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.

Yes, I was just reviewing those threads.  IIUC, the current proposal is
to adopt FreeBSD's version of indent as being less buggy and better
maintained than NetBSD's, and then patch it as necessary.

We'd put off the decision what to do exactly until a more suitable time,
but I think that time is now.  What I think we ought to do is go ahead
and indent the tree with current pgindent, and then we have a basis for
experimenting with replacement versions and seeing what they'll do
differently.  If we can make a decision and adopt any changes within
the next few weeks, I think that that timing will be about the least pain
we can hope for.

If we do anything with as much impact as changing the indentation rule
for string continuations, I will certainly vote for running the new
pgindent over the back branches too.  I still bear the scars of dealing
with the comment-reflowing changes that Bruce put in circa 8.1 --- that
broke just about every back-patching effort for the next five years.
I don't want to go through that again.
        regards, tom lane



On 2017-05-17 13:35:22 -0400, Tom Lane wrote:
> Not sure about actually incorporating it into our repo.  Doing so would
> make it easier for people to use, for sure, and the license seems to be
> regular 3-clause BSD, so that angle is OK.  But do we want to be carrying
> around another 150K of source code?

150k? Isn't it like 3-4k?

- Andres



Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Not sure about actually incorporating it into our repo.  Doing so would
>> make it easier for people to use, for sure, and the license seems to be
>> regular 3-clause BSD, so that angle is OK.  But do we want to be carrying
>> around another 150K of source code?

> The alternatives are

> 1. rely on the dead code we've been using so far (the old BSD indent
> patched with our Pg-specific tweaks), or

> 2. rely on someone else's upstream code -- in this case, FreeBSD's as
> patched by Piotr.

> Now that Piotr's is about to find a home, perhaps it's okay for us to
> rely on that one.  I just didn't like the idea of running something from
> Piotr's personal repo.

Well, "pg_bsd_indent is whatever you can find in the FreeBSD repo" is
not a rule that is going to work either.  We need to have a standardized
version that all developers can run and get the same results.  So I
think we'll either have a blessed tarball that we pass around (same
as we do now), or we'll put it into our own tree.  I don't really see
much downside to the latter except bloat.
        regards, tom lane



Andres Freund <andres@anarazel.de> writes:
> On 2017-05-17 13:35:22 -0400, Tom Lane wrote:
>> Not sure about actually incorporating it into our repo.  Doing so would
>> make it easier for people to use, for sure, and the license seems to be
>> regular 3-clause BSD, so that angle is OK.  But do we want to be carrying
>> around another 150K of source code?

> 150k? Isn't it like 3-4k?

The version we're using now is

$ tar xfz ~/archive/pg_bsd_indent-1.3.tar.gz 
$ wc pg_bsd_indent/*   38    122    928 pg_bsd_indent/Makefile  107    831   4835 pg_bsd_indent/README  508   1743
11988pg_bsd_indent/args.c  569   2727  14732 pg_bsd_indent/indent.1 1288   5677  37815 pg_bsd_indent/indent.c  101
671  4251 pg_bsd_indent/indent_codes.h  367   2376  15206 pg_bsd_indent/indent_globs.h  685   2781  18045
pg_bsd_indent/io.c 634   2709  17017 pg_bsd_indent/lexi.c  306   1133   8019 pg_bsd_indent/netbsd.patch  366   1659
10953pg_bsd_indent/parse.c  478   2427  15199 pg_bsd_indent/pr_comment.c 5447  24856 158988 total
 

(Note I was counting bytes not LOC.)

I've not looked at FreeBSD's version, but I'll bet a nickel it's bigger.
        regards, tom lane



On 2017-05-17 14:44:31 -0400, Tom Lane wrote:
> $ tar xfz ~/archive/pg_bsd_indent-1.3.tar.gz 
> $ wc pg_bsd_indent/*
>     38    122    928 pg_bsd_indent/Makefile
>    107    831   4835 pg_bsd_indent/README
>    508   1743  11988 pg_bsd_indent/args.c
>    569   2727  14732 pg_bsd_indent/indent.1
>   1288   5677  37815 pg_bsd_indent/indent.c
>    101    671   4251 pg_bsd_indent/indent_codes.h
>    367   2376  15206 pg_bsd_indent/indent_globs.h
>    685   2781  18045 pg_bsd_indent/io.c
>    634   2709  17017 pg_bsd_indent/lexi.c
>    306   1133   8019 pg_bsd_indent/netbsd.patch
>    366   1659  10953 pg_bsd_indent/parse.c
>    478   2427  15199 pg_bsd_indent/pr_comment.c
>   5447  24856 158988 total


> (Note I was counting bytes not LOC.)

Ah, that explains that ;)


> I've not looked at FreeBSD's version, but I'll bet a nickel it's bigger.

Not really, even counting the added tests.
 1275   5805  38674 freebsd_indent/indent.c  595   2867  15020 freebsd_indent/indent.1    9     25    240
freebsd_indent/Makefile 342   1451   9587 freebsd_indent/parse.c  343   2234  14637 freebsd_indent/indent_globs.h  356
1771  11311 freebsd_indent/pr_comment.c  522   2190  14063 freebsd_indent/io.c   18     48    361
freebsd_indent/Makefile.depend   7     18     89 freebsd_indent/tests/cppelsecom.0.stdout    0      1      5
freebsd_indent/tests/nsac.pro  52    173   1093 freebsd_indent/tests/comments.0    6     11     54
freebsd_indent/tests/nsac.0.stdout   0      1      4 freebsd_indent/tests/label.pro    8     20     96
freebsd_indent/tests/float.0.stdout   0      1      4 freebsd_indent/tests/sac.pro    9     20    127
freebsd_indent/tests/surplusbad.0.stdout  60    177   1127 freebsd_indent/tests/comments.0.stdout    0      1     22
freebsd_indent/tests/types_from_file.pro   7     18     86 freebsd_indent/tests/cppelsecom.0   30     46    284
freebsd_indent/tests/f_decls.0.stdout  23     50    571 freebsd_indent/tests/parens.0   14     35    246
freebsd_indent/tests/list_head.0.stdout  73    147    900 freebsd_indent/tests/declarations.0.stdout   16     35    242
freebsd_indent/tests/list_head.0  21     51    214 freebsd_indent/tests/struct.0    6     20     95
freebsd_indent/tests/float.0  42    118    555 freebsd_indent/tests/elsecomment.0   13     31    134
freebsd_indent/tests/label.0  23     50    558 freebsd_indent/tests/parens.0.stdout   27     51    286
freebsd_indent/tests/f_decls.0   9     20    125 freebsd_indent/tests/surplusbad.0    9     31    208
freebsd_indent/tests/binary.0   4     12     54 freebsd_indent/tests/nsac.0    0      1      4
freebsd_indent/tests/surplusbad.pro   4     12     54 freebsd_indent/tests/sac.0    1      3     15
freebsd_indent/tests/parens.pro   5     19     95 freebsd_indent/tests/offsetof.0    6     17    102
freebsd_indent/tests/wchar.0.stdout  47    118    578 freebsd_indent/tests/elsecomment.0.stdout   79    143    850
freebsd_indent/tests/declarations.0   3     14     60 freebsd_indent/tests/types_from_file.0    0      1      3
freebsd_indent/tests/elsecomment.pro   6     12     55 freebsd_indent/tests/sac.0.stdout    1      2      3
freebsd_indent/tests/types_from_file.list   6     17     94 freebsd_indent/tests/wchar.0    3     15     62
freebsd_indent/tests/types_from_file.0.stdout  11     31    209 freebsd_indent/tests/binary.0.stdout    7     19     96
freebsd_indent/tests/offsetof.0.stdout  14     31    207 freebsd_indent/tests/label.0.stdout    0      1      4
freebsd_indent/tests/comments.pro  23     47    215 freebsd_indent/tests/struct.0.stdout  100    818   4720
freebsd_indent/README  51    300   2082 freebsd_indent/indent.h  351   1415  10848 freebsd_indent/args.c   75    425
2740freebsd_indent/indent_codes.h  654   2577  17238 freebsd_indent/lexi.c 5366  23567 151406 total
 


- Andres



On 2017-05-17 20:31, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> On 2017-05-17 19:16, Alvaro Herrera wrote:
>>> We have someone who has studied the BSD indent code and even sent us
>>> patches to fix quite a few bugs, but we've largely ignored his efforts
>>> so far.  I propose we take that indent as part of our repo, and patch it
>>> to our preferences.
>
>> That, I assume, would be me. Coincidentally, I'm about to push my fixes
>> upstream (FreeBSD). Before that happens, my changes can be obtained from
>> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.
>
> Yes, I was just reviewing those threads.  IIUC, the current proposal is
> to adopt FreeBSD's version of indent as being less buggy and better
> maintained than NetBSD's, and then patch it as necessary.

One of my goals here is to fix bugs in FreeBSD indent so it's easier to
develop and maintain. I've also tried hard to not introduce serious
regressions for FreeBSD which also uses indent (for some sub-projects
even automatically). This is an ongoing effort.

What I describe below I believe to have been largely achieved.

Another goal is to incorporate all custom changes that make current
pg_bsd_indent yet another, unique indent fork, into FreeBSD indent - so
that maintaining patches for some other indent by the Postgres project
wouldn't be necessary. I understand the need to have control over a copy
of it within the Postgres project but it would be nice to not
effectively fork it by carrying custom patches around.

The third significant issue I kept in my mind was to shift some
work-arounds from pgindent to indent. When I use my indent as the base
for pgindent and set its options like this:
-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb
-cli1 -sac -ts4 -cp10
I can remove most of the work-arounds written in the perl script and
still get pretty decent results. But I expect some debate over a few things.




Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> That, I assume, would be me. Coincidentally, I'm about to push my fixes
> upstream (FreeBSD). Before that happens, my changes can be obtained from
> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.

Do you have recommendations for the switches to use with this to
match PG style?  It doesn't look like it takes quite the same
switches as the old netbsd code.
        regards, tom lane



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> The third significant issue I kept in my mind was to shift some
> work-arounds from pgindent to indent.

Yeah, I was wondering about that too.

> When I use my indent as the base
> for pgindent and set its options like this:
> -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb
> -cli1 -sac -ts4 -cp10

Ah, thanks, ignore the message I just sent asking for that ...

> I can remove most of the work-arounds written in the perl script and
> still get pretty decent results. But I expect some debate over a few things.

... but what parts of the perl script do you remove?  I'm trying
to duplicate whatever results you're currently getting.
        regards, tom lane



On 2017-05-17 22:11, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> The third significant issue I kept in my mind was to shift some
>> work-arounds from pgindent to indent.
>
> Yeah, I was wondering about that too.
>
>> When I use my indent as the base
>> for pgindent and set its options like this:
>> -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro -bbb
>> -cli1 -sac -ts4 -cp10
>
> Ah, thanks, ignore the message I just sent asking for that ...
>
>> I can remove most of the work-arounds written in the perl script and
>> still get pretty decent results. But I expect some debate over a few things.
>
> ... but what parts of the perl script do you remove?  I'm trying
> to duplicate whatever results you're currently getting.

Full copy of my pgindent attached.  Changes commented below.


@@ -17,7 +17,7 @@ my $devnull        = File::Spec->devnull;

 # Common indent settings
 my $indent_opts =
-  "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro
-bbb";
+  "-bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 -i4 -l79 -lp -nip -npro
-bbb -cli1 -sac -ts4 -cp10";

 # indent-dependent settings
 my $extra_opts = "";

The additional options are necessary. Mostly they replace the work-arounds.


@@ -198,60 +198,16 @@ sub pre_indent
 {
        my $source = shift;

-       # remove trailing whitespace
-       $source =~ s/[ \t]+$//gm;
-

I'm not sure there won't be any trailing white-space, but I've committed
changes that should limit it.


        ## Comments

        # Convert // comments to /* */
        $source =~ s!^([ \t]*)//(.*)$!$1/* $2 */!gm;

-       # 'else' followed by a single-line comment, followed by
-       # a brace on the next line confuses BSD indent, so we push
-       # the comment down to the next line, then later pull it
-       # back up again.  Add space before _PGMV or indent will add
-       # it for us.
-       # AMD: A symptom of not getting this right is that you see
errors like:
-       # FILE: ../../../src/backend/rewrite/rewriteHandler.c
-       # Error@2259:
-       # Stuff missing from end of file
-       $source =~
-         s!(\}|[ \t])else[ \t]*(/\*)(.*\*/)[ \t]*$!$1else\n    $2
_PGMV$3!gm;
-
-       # Indent multi-line after-'else' comment so BSD indent will move it
-       # properly. We already moved down single-line comments above.
-       # Check for '*' to make sure we are not in a single-line comment
that
-       # has other text on the line.
-       $source =~ s!(\}|[ \t])else[ \t]*(/\*[^*]*)[ \t]*$!$1else\n
$2!gm;

These are definitely fixed.


        ## Other

-       # Work around bug where function that defines no local variables
-       # misindents switch() case lines and line after #else.  Do not do
-       # for struct/enum.
-       my @srclines = split(/\n/, $source);
-       foreach my $lno (1 .. $#srclines)
-       {
-               my $l2 = $srclines[$lno];
-
-               # Line is only a single open brace in column 0
-               next unless $l2 =~ /^\{[ \t]*$/;
-
-               # previous line has a closing paren
-               next unless $srclines[ $lno - 1 ] =~ /\)/;
-
-               # previous line was struct, etc.
-               next
-                 if $srclines[ $lno - 1 ] =~
-                         m!=|^(struct|enum|[ \t]*typedef|extern[ \t]+"C")!;
-
-               $srclines[$lno] = "$l2\nint pgindent_func_no_var_fix;";
-       }
-       $source = join("\n", @srclines) . "\n";    # make sure there's a
final \n
-

I don't have time now to double-check, but the above shouldn't be needed
anymore.


-       # Pull up single-line comment after 'else' that was pulled down
above
-       $source =~ s!else\n[ \t]+/\* _PGMV!else\t/*!g;
-
-       # Indent single-line after-'else' comment by only one tab.
-       $source =~ s!(\}|[ \t])else[ \t]+(/\*.*\*/)[ \t]*$!$1else\t$2!gm;
-
-       # Add tab before comments with no whitespace before them (on a
tab stop)
-       $source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm;
-
-       # Remove blank line between opening brace and block comment.
-       $source =~ s!(\t*\{\n)\n([ \t]+/\*)$!$1$2!gm;
-

These are almost definitely fixed in indent.


-       # cpp conditionals
-
-       # Reduce whitespace between #endif and comments to one tab
-       $source =~ s!^\#endif[ \t]+/\*!#endif   /*!gm;
-
        ## Functions

-       # Work around misindenting of function with no variables defined.
-       $source =~ s!^[ \t]*int[ \t]+pgindent_func_no_var_fix;[
\t]*\n{1,2}!!gm;
-

These have likely been fixed.


-       ## Other
-
-       # Remove too much indenting after closing brace.
-       $source =~ s!^\}\t[ \t]+!}\t!gm;
-
-       # Workaround indent bug that places excessive space before 'static'.
-       $source =~ s!^static[ \t]+!static !gm;
-
-       # Remove leading whitespace from typedefs
-       $source =~ s!^[ \t]+typedef enum!typedef enum!gm
-         if $source_filename =~ 'libpq-(fe|events).h$';

I believe these have been fixed as well.


-       # Remove trailing blank lines
-       $source =~ s!\n+\z!\n!

I'm not sure, but shouldn't be necessary.


@@ -541,7 +462,6 @@ foreach my $source_filename (@files)

        # Protect backslashes in DATA() and wrapping in CATALOG()

-       $source = detab($source);
        $source =~ s!^((DATA|CATALOG)\(.*)$!/*$1*/!gm;

        $source = run_indent($source, \$error_message);
@@ -553,7 +473,6 @@ foreach my $source_filename (@files)

  # Restore DATA/CATALOG lines; must be done here so tab alignment is
preserved
        $source =~ s!^/\*((DATA|CATALOG)\(.*)\*/$!$1!gm;
-       $source = entab($source);

Definitely unnecessary if you use indent -ts4. There are slight differences.


Sorry for the delay, I was unprepared for answering this tonight.

-- 
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] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Piotr Stefaniak
Дата:
On 2017-05-17 17:55, Peter Eisentraut wrote:
> On 5/17/17 10:14, Tom Lane wrote:
>> What I was concerned about was that pgindent will reindent the second
>> line so that it's impossible to tell whether the spacing is correct.
>
> pgindent moving string continuations to the left is a completely
> terrible behavior anyway and we should look into changing that.  Just
> look at the mess it makes out of SELECT queries in pg_dump.c.

If I remember correctly, it tries to right-align string literals to
whatever -l ("Maximum length of an output line") was set to.



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Alvaro Herrera
Дата:
Piotr Stefaniak wrote:
> On 2017-05-17 17:55, Peter Eisentraut wrote:
> > On 5/17/17 10:14, Tom Lane wrote:
> >> What I was concerned about was that pgindent will reindent the second
> >> line so that it's impossible to tell whether the spacing is correct.
> > 
> > pgindent moving string continuations to the left is a completely
> > terrible behavior anyway and we should look into changing that.  Just
> > look at the mess it makes out of SELECT queries in pg_dump.c.
> 
> If I remember correctly, it tries to right-align string literals to
> whatever -l ("Maximum length of an output line") was set to.

Yeah, it does that (for error messages too).

I'm not sure what's the behavior we do want.  One choice is that the
continuation string opening quote should line up with the opening quote
in the previous line.  So for instance:
   elog(ERROR, "this message is very long"               "and the second line may get over the 80 chars limit if we let
it");

but I suppose some we would like the opening quote to line up with the
parens:
   elog(ERROR, "this message is very long "        "and the second line may get over the 80 chars limit if we let
it");

I vote that the first form is best, even if it would cause me some pain
because my vim config doesn't line up continuations like that. (Not sure
I can tweak it to work that way.)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> Full copy of my pgindent attached.  Changes commented below.

Thanks!  I ran this, along with the indent copy I pulled from your
github repo a couple hours ago, over the current PG tree (post
Bruce's run).  I got a diff extending to about 100K lines :-(
which I will not post here.  It seemed like a very large fraction
of that was that old pgindent chooses to use a space rather than
a tab if the tab would only move over one column.  This version
uses a tab anyway.

I hacked around that by putting back a detab/entab step at the end
using the existing subroutines in pgindent.  That about halved the
size of the diff, but it's still too big to post.  Much of what
I'm seeing with this version is randomly different decisions about
how far to indent comments, eg

@@ -101,8 +101,8 @@ typedef struct BloomOptions{   int32       vl_len_;        /* varlena header (do not touch
directly!)*/   int         bloomLength;    /* length of signature in words (not bits!) */
 
-   int         bitSize[INDEX_MAX_KEYS];        /* # of bits generated for
-                                                * each index key */
+   int         bitSize[INDEX_MAX_KEYS];    /* # of bits generated for each
+                                            * index key */} BloomOptions;/*

(I untabified the above fragment in the hope of making it more readable
in email.)

It does seem to be handling formatting around sizeof() calls a lot better
than the old code, as well as function pointer typedefs.  So those are
huge wins.  But can we avoid the changes mentioned above?  I'd like the
new version to only differ in ways that are clear improvements.
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Piotr Stefaniak wrote:
>> If I remember correctly, it tries to right-align string literals to
>> whatever -l ("Maximum length of an output line") was set to.

> Yeah, it does that (for error messages too).

Piotr's version seems to at least do this more consistently than the
old version; for instance I notice this diff from Bruce's run:

@@ -1864,8 +1864,8 @@ describeOneTableDetails(const char *schemaname,       if (verbose)
printfPQExpBuffer(&buf,                            "SELECT inhparent::pg_catalog.regclass,"
 
-                           "       pg_get_expr(c.relpartbound, inhrelid),"
-                         "     pg_get_partition_constraintdef(inhrelid)"
+                             "     pg_get_expr(c.relpartbound, inhrelid),"
+                             "     pg_get_partition_constraintdef(inhrelid)"                             " FROM
pg_catalog.pg_classc"                             " JOIN pg_catalog.pg_inherits"                             " ON c.oid
=inhrelid"
 

(Again, untabified for clarity.)  However, it didn't do anything to any of
the horribly-formatted queries in pg_dump.c, so it's mostly following the
same rule as before.

> I'm not sure what's the behavior we do want.  One choice is that the
> continuation string opening quote should line up with the opening quote
> in the previous line.  So for instance:

Yeah, I'd vote for that one too.  If you want to line things up with
a function call paren, you can always start the whole literal on a fresh
line, as in the above example.
        regards, tom lane



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.

От
Tom Lane
Дата:
Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> That, I assume, would be me. Coincidentally, I'm about to push my fixes
> upstream (FreeBSD). Before that happens, my changes can be obtained from
> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.

I spent a little bit of time on portability testing, because we are
certainly going to insist that this tool be portable to more than
just FreeBSD.  Things are not very good as it stands:

* Makefile is 100% BSD-specific.  Possibly we could just agree to
disagree on that point, and include a PG-style makefile that is not
like upstream's.  I attach the one I used for test purposes.

* __FBSDID() macros fail to compile anywhere else than FreeBSD.
Couldn't you hide those inside #if 0, as you're already doing for
the ancient sccsid strings?

* Please put the copyright[] string in indent.c inside #if 0 too,
as that draws unreferenced-variable warnings on some compilers.

* There's one use of bcopy(), please replace with memmove().

* References to <sys/cdefs.h> and <err.h> are problematic, as both
are BSD-isms not found in POSIX.  They seem to be available anyway
on Linux, but I bet not on Windows or SysV-tradition boxes.
I removed the <sys/cdefs.h> includes and didn't see any problems,
but removing <err.h> exposes calls to err() and errx(), which
we'd have to find replacements for.  Maybe just change them to
standard-conforming printf() + exit()?

            regards, tom lane

#-------------------------------------------------------------------------
#
# Makefile for src/tools/freebsd_indent
#
# Copyright (c) 2003-2017, PostgreSQL Global Development Group
#
# src/tools/freebsd_indent/Makefile
#
#-------------------------------------------------------------------------

subdir = src/tools/freebsd_indent
top_builddir = ../../..
include $(top_builddir)/src/Makefile.global

override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)

OBJS= args.o indent.o io.o lexi.o parse.o pr_comment.o

all: freebsd_indent

freebsd_indent: $(OBJS)
    $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) $(LDFLAGS_EX) -o $@$(X)

install: all installdirs
    $(INSTALL_PROGRAM) freebsd_indent$(X) '$(DESTDIR)$(bindir)/freebsd_indent$(X)'

installdirs:
    $(MKDIR_P) '$(DESTDIR)$(bindir)'

uninstall:
    rm -f '$(DESTDIR)$(bindir)/freebsd_indent$(X)'

clean distclean maintainer-clean:
    rm -f freebsd_indent$(X) $(OBJS)

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017-05-17 23:46, Tom Lane wrote:
> I hacked around that by putting back a detab/entab step at the end
> using the existing subroutines in pgindent.  That about halved the
> size of the diff, but it's still too big to post.  Much of what
> I'm seeing with this version is randomly different decisions about
> how far to indent comments

pgindent doesn't set the -c indent option ("The column    in which comments
on code start."), so indent uses the default value of 33 (meaning column
32). If the code pushes the comment further than column 32, indent only
places a single tab between the two just to separate them.

This, given 4-column tabs, should result in placing the comment on
bitSize[INDEX_MAX_KEYS]; from your example onto column 44 - which the
newer version of indent does (if you tell it -ts4), unlike the older
one. I think that this is an improvement.

> It does seem to be handling formatting around sizeof() calls a lot better
> than the old code, as well as function pointer typedefs.  So those are
> huge wins.  But can we avoid the changes mentioned above?  I'd like the
> new version to only differ in ways that are clear improvements.

I don't know how to avoid the improvement. Try removing -ts4 as well as
putting back detab+entab.



Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advanceof pgindent run.

От
Piotr Stefaniak
Дата:
On 2017-05-18 18:13, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> That, I assume, would be me. Coincidentally, I'm about to push my fixes
>> upstream (FreeBSD). Before that happens, my changes can be obtained from
>> https://github.com/pstef/freebsd_indent and tested, if anyone wishes.
>
> I spent a little bit of time on portability testing, because we are
> certainly going to insist that this tool be portable to more than
> just FreeBSD.  Things are not very good as it stands:
>
> * Makefile is 100% BSD-specific.  Possibly we could just agree to
> disagree on that point, and include a PG-style makefile that is not
> like upstream's.  I attach the one I used for test purposes.

This would have to live outside of FreeBSD for obvious (or not) reasons.
Most likely as a part of pg_bsd_indent.  I use the original ("BSD")
Makefile on Linux, feeding it to bmake(1). But I don't expect bmake to
become a requirement for pg_bsd_indent.

> * __FBSDID() macros fail to compile anywhere else than FreeBSD.
> Couldn't you hide those inside #if 0, as you're already doing for
> the ancient sccsid strings?

The use of __FBSDID macro won't be going anywhere from FreeBSD indent,
I'm afraid. But I think it could be if-def'd out under systems other
than FreeBSD.

> * Please put the copyright[] string in indent.c inside #if 0 too,
> as that draws unreferenced-variable warnings on some compilers.
>
> * There's one use of bcopy(), please replace with memmove().

These could probably be done upstream. I'd like to convert the strings
to comments.

> * References to <sys/cdefs.h> and <err.h> are problematic, as both
> are BSD-isms not found in POSIX.  They seem to be available anyway
> on Linux, but I bet not on Windows or SysV-tradition boxes.
> I removed the <sys/cdefs.h> includes and didn't see any problems,
> but removing <err.h> exposes calls to err() and errx(), which
> we'd have to find replacements for.  Maybe just change them to
> standard-conforming printf() + exit()?

I'll look into why indent includes sys/cdefs.h.  I don't expect to be
allowed to replace err() and errx() with equivalent solutions based on
ISO C and POSIX. I fear the idea of detecting err*() support prior to
compilation... Probably a much simpler solution would be to replace
err*() with equivalent solutions in the PG's fork of indent.  printf()
and exit() would probably be insufficient, you'd also need strerror(), I
think.



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-05-17 23:46, Tom Lane wrote:
>> ... Much of what
>> I'm seeing with this version is randomly different decisions about
>> how far to indent comments

> pgindent doesn't set the -c indent option ("The column in which comments
> on code start."), so indent uses the default value of 33 (meaning column
> 32). If the code pushes the comment further than column 32, indent only
> places a single tab between the two just to separate them.

Well, actually what it does is to push the comment to what it thinks is
the next tab stop.  So the core issue here is that the comments get
formatted differently for -ts4 than -ts8.  I think that's arguably a bug;
the width of tabs should only affect how whitespace is stored, not
formatting decisions.  Don't suppose you'd like to introduce a separate
parameter that defines the extra-indentation step for comments?

> This, given 4-column tabs, should result in placing the comment on
> bitSize[INDEX_MAX_KEYS]; from your example onto column 44 - which the
> newer version of indent does (if you tell it -ts4), unlike the older
> one. I think that this is an improvement.

It may or may not be an improvement, but right now what I want is to see
what this version of indent does differently, with as few extraneous
changes as possible.  We can argue later about whether we're willing to
accept gratuitous comment reformatting, but when one can't even find the
positive changes in amongst the noise, the chances of getting this accepted
are not good.

> I don't know how to avoid the improvement. Try removing -ts4 as well as
> putting back detab+entab.

I tried that but it did not produce as good a match to the old results as
what I'd previously arrived at by trial and error, which was to hack
pr_comment() like this:

@@ -148,7 +151,9 @@ pr_comment(void)        else        ps.com_col = ps.decl_on_line || ps.ind_level == 0            ?
ps.decl_com_ind: ps.com_ind;
 
-        if (ps.com_col <= target_col)
+        if (ps.com_col < target_col)
+        ps.com_col = 8 * (1 + (target_col - 1) / 8) + 1;
+        else if (ps.com_col == target_col)        ps.com_col = tabsize * (1 + (target_col - 1) / tabsize) + 1;
if(ps.com_col + 24 > adj_max_col)        adj_max_col = ps.com_col + 24;
 

I'm not really sure why the old behavior seems to be to move only 4 spaces
when right at the boundary, but there you have it.

I also found that there was extra spacing getting inserted for some cases
like
case afairlylonglabel:    /* comment */

which I eventually tracked down to the fact that this bit:
    /*     * turn everything so far into a label     */    {    int len = e_code - s_code;
    CHECK_SIZE_LAB(len + 3);    memcpy(e_lab, s_code, len);    e_lab += len;    *e_lab++ = ':';    *e_lab++ = ' ';
*e_lab= '\0';    e_code = s_code;    }
 

is inserting an extra space into the "lab" string, causing pr_comment()
to think that the label extends one character to the right of where
it really does, so that it moves the comment over when it need not.
I am not sure why it's like that, but compensating for it in pr_comment()
like this improved matters:

@@ -137,8 +136,12 @@ pr_comment(void)        target_col = count_spaces(compute_code_target(), s_code);        else {
   target_col = 1;
 
-        if (s_lab != e_lab)
+        if (s_lab != e_lab) {            target_col = count_spaces(compute_label_target(), s_lab);
+            /* ignore any trailing space in lab for this purpose */
+            if (e_lab[-1] == ' ')
+            target_col--;
+        }        }        if (s_lab != e_lab && s_lab[1] == 'e' &&        (strncmp(s_lab, "#endif", 6) == 0 ||

(I see that the extra space after colon is inserted by the old version
of indent too, which makes it even less clear why the boundary-case
behavior is like this.  I have a feeling that this is hacking things
at the wrong place.)

That got me to a point where there was little enough noise that I could
start to see the real changes, and I soon noticed that there was a fair
amount of apparently buggy behavior, like this change:

diff --git a/contrib/pg_prewarm/pg_prewarm.c b/contrib/pg_prewarm/pg_prewarm.c
index 78d71ab..630bacc 100644
--- a/contrib/pg_prewarm/pg_prewarm.c
+++ b/contrib/pg_prewarm/pg_prewarm.c
@@ -33,8 +33,8 @@ PG_FUNCTION_INFO_V1(pg_prewarm);typedef enum{    PREWARM_PREFETCH,
-    PREWARM_READ,
-    PREWARM_BUFFER
+        PREWARM_READ,
+        PREWARM_BUFFER} PrewarmType;static char blockbuffer[BLCKSZ];

Curiously, there are other enum declarations that don't get the phony
extra indentation.  I traced through it a bit and eventually found that
the difference between OK and not OK is that the declarations that don't
get messed up look like "typedef enum enumlabel ...", ie the problem
with this one is there's no extra identifier after "enum".  The proximate
cause of that is this line in indent.c:
        ps.in_decl = ps.decl_on_line = ps.last_token != type_def;

which I see used to be just
    ps.in_decl = ps.decl_on_line = true;

in old indent.  This results in leaving ps.in_decl in the wrong state
when there's no enum label.  I suspect that this change is also accounting
for a lot of noise changes I see in struct decls that lack a label after
"typedef struct".  I don't know what it is you had in mind to fix with
this change, but surely there are a lot of other cases where the distance
back to the "typedef" keyword is variable?  (I tried reverting this
change, and it fixed the enum problem but introduced other issues.)

I'm also noticing some random-looking changes like this one:

diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c
index c3c10e1..c60369e 100644
--- a/contrib/isn/isn.c
+++ b/contrib/isn/isn.c
@@ -531,7 +531,7 @@ static boolean2string(ean13 ean, bool errorOK, char *result, bool shortType){    const char
*(*TABLE)[2];
-    const unsigned (*TABLE_index)[2];
+    const unsigned(*TABLE_index)[2];    enum isn_type type = INVALID;    char       *aux;

which is surely not an improvement; but I ran out of energy before
investigating that.
        regards, tom lane



I wrote:
> Curiously, there are other enum declarations that don't get the phony
> extra indentation.  I traced through it a bit and eventually found that
> the difference between OK and not OK is that the declarations that don't
> get messed up look like "typedef enum enumlabel ...", ie the problem
> with this one is there's no extra identifier after "enum".  The proximate
> cause of that is this line in indent.c:

>       ps.in_decl = ps.decl_on_line = ps.last_token != type_def;

I found that things seem to work better with this change:

@@ -939,7 +938,7 @@ check_type:       }       ps.in_or_st = true; /* this might be a structure or initialization
       * declaration */ 
-       ps.in_decl = ps.decl_on_line = ps.last_token != type_def;
+       ps.in_decl = ps.decl_on_line = (ps.last_token != type_def || type_code == structure);       if ( /*
!ps.in_or_st&& */ ps.dec_nest <= 0)       ps.just_saw_decl = 2;       prefix_blankline_requested = 0; 

I have no idea if that's a correct or sufficient fix, but it makes the
oddnesses around unnamed enum and struct declarations in the PG sources
go away.

There remain a small number of cases that look like bugs.  One is the
case I showed before:

@@ -531,7 +531,7 @@ static boolean2string(ean13 ean, bool errorOK, char *result, bool shortType){   const char
*(*TABLE)[2];
-   const unsigned (*TABLE_index)[2];
+   const unsigned(*TABLE_index)[2];   enum isn_type type = INVALID;   char       *aux;

I have an impression that this might be related to the ps.in_decl business,
but the patch shown above did not change it.  Possibly related is this:

diff --git a/src/include/access/gin_private.h b/src/include/access/gin_private.h
index 986fe6e..a2d11e5 100644
--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -149,8 +149,8 @@ typedef struct GinBtreeData   bool        (*findItem) (GinBtree, GinBtreeStack *);   /* insert
methods*/ 
-   OffsetNumber (*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
-   GinPlaceToPageRC (*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page
*);
+   OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
+   GinPlaceToPageRC(*beginPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void **, Page *, Page
*);  void        (*execPlaceToPage) (GinBtree, Buffer, GinBtreeStack *, void *, BlockNumber, void *);   void
*(*prepareDownlink)(GinBtree, Buffer);   void        (*fillRoot) (GinBtree, Page, BlockNumber, Page, BlockNumber,
Page);

Whatever's going on there, it seems to affect only a very small number
of places.  No idea why.

Also, multiple comments on the same line are now run together, which
is surely not better:

@@ -2144,11 +2144,11 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)        */       NewPage->xlp_magic =
XLOG_PAGE_MAGIC;
-       /* NewPage->xlp_info = 0; */    /* done by memset */
+       /* NewPage->xlp_info = 0; *//* done by memset */       NewPage->xlp_tli = ThisTimeLineID;
NewPage->xlp_pageaddr= NewPageBeginPtr; 
-       /* NewPage->xlp_rem_len = 0; */ /* done by memset */
+       /* NewPage->xlp_rem_len = 0; *//* done by memset */       /*        * If online backup is not in progress, mark
theheader to indicate 

Also, I found two places where an overlength comment line is simply busted
altogether --- notice that a character is missing at the split point:

@@ -257,7 +257,8 @@ get_pgpid(bool is_status_request)       /*        * The Linux Standard Base Core Specification 3.1
saysthis should        * return '4, program or service status is unknown' 
-        * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
+        * https://refspecs.linu
+        * base.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g        * eneric/iniscrptact.html        */
exit(is_status_request? 4 : 1); 

@@ -629,7 +629,8 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)                   /*
               * This code erroneously assumes '\.' on a line alone                    * inside a quoted CSV string
terminatesthe \copy. 
-                    * http://www.postgresql.org/message-id/E1TdNVQ-0001ju-GO@w
+                    * http://w
+                    * w.postgresql.org/message-id/E1TdNVQ-0001ju-GO@w                    * rigleys.postgresql.org
             */                   if (strcmp(buf, "\\.\n") == 0 || 

Another problem is that the handling of unrecognized typedef names as
field types in a struct has gotten significantly worse:

diff --git a/src/include/lib/simplehash.h b/src/include/lib/simplehash.h
index a35addf..919d262 100644
--- a/src/include/lib/simplehash.h
+++ b/src/include/lib/simplehash.h
@@ -114,14 +114,14 @@ typedef struct SH_TYPE   uint32      grow_threshold;   /* hash buckets */
-   SH_ELEMENT_TYPE *data;
+               SH_ELEMENT_TYPE * data;   /* memory context to use for allocations */   MemoryContext ctx;

I don't mind the space after the star so much, but the indentation
is awful.  In this particular example we *cannot* fix it by making
SH_ELEMENT_TYPE a real typedef, because it's actually meant to be
a macro and vary from one inclusion of simplehash.h to another.

I guess a possible answer is to create a list of "manual" typedef
names to add to the ones extracted from the buildfarm.  But if
it's possible for indent to be a bit more fail-soft in this type
of case, we'd not have to.  It seems like other users of indent
might have more need for plausible treatment of unknown typedef
names than we do, too.
        regards, tom lane



I wrote:
> Also, I found two places where an overlength comment line is simply busted
> altogether --- notice that a character is missing at the split point:

I found the cause of that: you need to apply this patch:

--- freebsd_indent/pr_comment.c~    2017-05-17 14:59:31.548442801 -0400
+++ freebsd_indent/pr_comment.c    2017-05-20 20:51:16.447332977 -0400
@@ -344,8 +353,8 @@ pr_comment(void)        {            int len = strlen(t_ptr);
-            CHECK_SIZE_COM(len);
-            memmove(e_com, t_ptr, len);
+            CHECK_SIZE_COM(len + 1);
+            memmove(e_com, t_ptr, len + 1);            last_bl = strpbrk(e_com, " \t");            e_com += len;
}
 

As the code stands, the strpbrk call is being applied to a
not-null-terminated string and therefore is sometimes producing an
insane value of last_bl, messing up decisions later in the comment.
Having the memmove include the trailing \0 resolves that.
        regards, tom lane



On 2017-05-21 03:00, Tom Lane wrote:
> I wrote:
>> Also, I found two places where an overlength comment line is simply busted
>> altogether --- notice that a character is missing at the split point:
>
> I found the cause of that: you need to apply this patch:
>
> --- freebsd_indent/pr_comment.c~    2017-05-17 14:59:31.548442801 -0400
> +++ freebsd_indent/pr_comment.c    2017-05-20 20:51:16.447332977 -0400
> @@ -344,8 +353,8 @@ pr_comment(void)
>          {
>              int len = strlen(t_ptr);
>
> -            CHECK_SIZE_COM(len);
> -            memmove(e_com, t_ptr, len);
> +            CHECK_SIZE_COM(len + 1);
> +            memmove(e_com, t_ptr, len + 1);
>              last_bl = strpbrk(e_com, " \t");
>              e_com += len;
>          }
>
> As the code stands, the strpbrk call is being applied to a
> not-null-terminated string and therefore is sometimes producing an
> insane value of last_bl, messing up decisions later in the comment.
> Having the memmove include the trailing \0 resolves that.

I have been analyzing this and came to different conclusions. Foremost,
a strpbrk() call like that finds the first occurrence of either space or
a tab, but last_bl means "last blank" - it's used for marking where to
wrap a comment line if it turns out to be too long. The previous coding
moved the character sequence byte after byte, updating last_bl every
time it was copying one of the two characters. I've rewritten that part as:                   CHECK_SIZE_COM(len);
            memmove(e_com, t_ptr, len); 
-                   last_bl = strpbrk(e_com, " \t");                   e_com += len;
+                   last_bl = NULL;
+                   for (t_ptr = e_com - 1; t_ptr > e_com - len; t_ptr--)
+                       if (*t_ptr == ' ' || *t_ptr == '\t') {
+                           last_bl = t_ptr;
+                           break;
+                       }               }


But then I also started to wonder if there is any case when there's more
than one character to copy and I haven't found one yet. It looks like    } while (!memchr("*\n\r\b\t", *buf_ptr, 6) &&
 (now_col <= adj_max_col || !last_bl)); 
guarantees that if we're past adj_max_col, it'll only be one non-space
character. But I'm not sure yet.




Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-05-21 03:00, Tom Lane wrote:
>> I wrote:
>>> Also, I found two places where an overlength comment line is simply busted
>>> altogether --- notice that a character is missing at the split point:

>> I found the cause of that: you need to apply this patch:

> I have been analyzing this and came to different conclusions.

Well, the code as it stands breaks those two comments (and a third one
I'd failed to notice before).  With the patch I propose, the only changes
are that those comments are left unmolested.  So even aside from the
fact that this code is visibly unsafe, it does correspond to the symptom.
        regards, tom lane



On Sun, May 21, 2017 at 10:12:20AM -0400, Tom Lane wrote:
> Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> > On 2017-05-21 03:00, Tom Lane wrote:
> >> I wrote:
> >>> Also, I found two places where an overlength comment line is simply busted
> >>> altogether --- notice that a character is missing at the split point:
> 
> >> I found the cause of that: you need to apply this patch:
> 
> > I have been analyzing this and came to different conclusions.
> 
> Well, the code as it stands breaks those two comments (and a third one
> I'd failed to notice before).  With the patch I propose, the only changes
> are that those comments are left unmolested.  So even aside from the
> fact that this code is visibly unsafe, it does correspond to the symptom.

Frankly, I found it ironic that the BSD indent code, which was designed
to improve code clarity, was so confusingly written.  I went with the
sed script (and later Perl script) wrapper solution because the BSD
indent code was so confusing to me.

It seems like a "The Cobbler's children have no shoes" syndrome:
https://english.stackexchange.com/questions/159004/the-cobblers-children-have-no-shoes

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +



> * const unsigned(*TABLE_index)[2];
> * OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
> * an overlength comment line is simply busted altogether

Fixed and pushed to my github repository. Note that indent won't wrap
long lines like links or paths anymore. But obviously it can't join
parts of long links that have been wrapped by previous pgindent runs.

> * the comments get formatted differently for -ts4 than -ts8
> * extra spacing getting inserted for fairly long labels
> * some enum declarations get the phony extra indentation
> * comments on the same line are now run together
> * surplus indentation for SH_ELEMENT_TYPE * data;

Please tell me if the list of your complaints above is incomplete. I
will analyze those next.



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> * const unsigned(*TABLE_index)[2];
>> * OffsetNumber(*findChildPtr) (GinBtree, Page, BlockNumber, OffsetNumber);
>> * an overlength comment line is simply busted altogether

> Fixed and pushed to my github repository.

I'm pretty confused by your github repo.  It doesn't have a master
branch, and what seems to be the HEAD branch is called "pass3", but
when I tried to "git pull" just now I got


From https://github.com/pstef/freebsd_indent+ b7b74cb...e4ccc02 pass3      -> origin/pass3  (forced update)
First, rewinding head to replay your work on top of it...
Applying: [bugfix] Don't add unneeded space in function pointer declaration.
Using index info to reconstruct a base tree...
M       indent.c
M       tests/declarations.0.stdout
Falling back to patching base and 3-way merge...
Auto-merging indent.c
CONFLICT (content): Merge conflict in indent.c
Failed to merge in the changes.
Patch failed at 0001 [bugfix] Don't add unneeded space in function pointer declaration.
The copy of the patch that failed is found in:  /home/postgres/freebsd_indent/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".


... which is pretty odd because there were no local changes.  It should
have just done a fast-forward, I'd think.

Being lazy, I just wiped my copy and re-cloned, but it still seems the
same as before ... last commit on the pass3 branch is from Mar 4.
What branch should I be paying attention to?

> Note that indent won't wrap
> long lines like links or paths anymore. But obviously it can't join
> parts of long links that have been wrapped by previous pgindent runs.

Check, I wouldn't expect it to.  I'll probably make a manual pass to sew
split-up URLs in comments back together.

>> * the comments get formatted differently for -ts4 than -ts8
>> * extra spacing getting inserted for fairly long labels
>> * some enum declarations get the phony extra indentation
>> * comments on the same line are now run together
>> * surplus indentation for SH_ELEMENT_TYPE * data;

> Please tell me if the list of your complaints above is incomplete. I
> will analyze those next.

There's also the portability issues: __FBSDID() and bcopy() and
<sys/cdefs.h>.
        regards, tom lane



On 2017-05-22 01:50, Tom Lane wrote:
> Being lazy, I just wiped my copy and re-cloned, but it still seems the
> same as before ... last commit on the pass3 branch is from Mar 4.
> What branch should I be paying attention to?

Sorry for the trouble, this is because I interactively git-rebased it in
order to rewrite history. I do this to limit the number of commits to
the FreeBSD repository. Next time I'll leave fix-ups in chronological
order and meld them with what they fix just before committing to the
FreeBSD repository.

pass3 is the right branch. A fresh clone should have worked as in the
attached log.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Вложения
Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-05-22 01:50, Tom Lane wrote:
>> Being lazy, I just wiped my copy and re-cloned, but it still seems the
>> same as before ... last commit on the pass3 branch is from Mar 4.
>> What branch should I be paying attention to?

> pass3 is the right branch. A fresh clone should have worked as in the
> attached log.

Ah, I now see that the code did change, I'd just been confused by
the "git log" history.

Small thought: shouldn't your updated code in pr_comment be changed to

-                   for (t_ptr = e_com + len - 1; t_ptr > e_com; t_ptr--)
+                   for (t_ptr = e_com + len - 1; t_ptr >= e_com; t_ptr--)

Perhaps it's impossible for the character right at e_com to be a space,
but there seems no need for this loop to assume that.
        regards, tom lane



>>> * the comments get formatted differently for -ts4 than -ts8

Still haven't put any thought into it, so I still don't know what to do
here.

>>> * extra spacing getting inserted for fairly long labels

I think the fix is as easy as not producing the space. I committed that.

>>> * some enum declarations get the phony extra indentation
>>> * surplus indentation for SH_ELEMENT_TYPE * data;

I think this is now fixed.

>>> * comments on the same line are now run together
Indent has worked like for a long time; current pg_bsd_indent does that
as well. It was now uncovered by my removing this line from pgindent:
# Add tab before comments with no whitespace before them (on a tab stop)$source =~ s!(\S)(/\*.*\*/)$!$1\t$2!gm;

> There's also the portability issues: __FBSDID() and bcopy() and
> <sys/cdefs.h> [and err.h].

I think that's fixed as well.


I've never been too excited to improve indent and it's increasingly
challenging for me to force myself to work on it now, after I've
invested so much of my spare time into it. So please bear with me if
there are any errors.

On Sun, Jun 11, 2017 at 09:14:36PM +0000, Piotr Stefaniak wrote:
> I've never been too excited to improve indent and it's increasingly
> challenging for me to force myself to work on it now, after I've
> invested so much of my spare time into it. So please bear with me if
> there are any errors.

Understood.  You would think that with the number of open-source
programs written in C that there would be more interest in C formatting
tools.  Is the Postgres community the only ones with specific
requirements, or is it just that we settled on an older tool and can't
easily change?  I have reviewed the C formatting options a few times
over the years and every time the other options were worse than what we
had.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
>> There's also the portability issues: __FBSDID() and bcopy() and
>> <sys/cdefs.h> [and err.h].

> I think that's fixed as well.

I just finished some preliminary portability testing and things look
much improved.  The Makefile is still BSD-ish of course, but I think
we'll just agree to disagree there.  The only thing I could find to
quibble about is that on old macOS versions I get

In file included from indent.c:49:
indent_globs.h:222:1: warning: "STACKSIZE" redefined
In file included from /usr/include/machine/param.h:30,                from /usr/include/sys/param.h:104,
fromindent.c:42: 
/usr/include/ppc/param.h:53:1: warning: this is the location of the previous definition

Maybe you could rename that symbol to IND_STACKSIZE or some such?

Also, I am wondering about the test cases under tests/.  I do not
see anything in the Makefile or elsewhere suggesting how those are
to be used.  It would sure be nice to have some quick smoke-test
to check that a build on a new platform is working.
        regards, tom lane



I've now done a round of comparisons of results of our old indent
with your current version.  There's still one serious bug in the latter:
it continues to misformat enum typedefs, for instance

*************** PG_FUNCTION_INFO_V1(pg_prewarm);
*** 33,40 **** typedef enum {     PREWARM_PREFETCH,
!     PREWARM_READ,
!     PREWARM_BUFFER } PrewarmType;  static char blockbuffer[BLCKSZ];
--- 33,40 ---- typedef enum {     PREWARM_PREFETCH,
!         PREWARM_READ,
!         PREWARM_BUFFER } PrewarmType;  static char blockbuffer[BLCKSZ];

I spent some time trying to diagnose that, and what I found is that
while it's scanning the enum list, ps.in_decl is false, which causes
dump_line() to set ps.ind_stmt to true after the first line, which
causes later calls of compute_code_target() to add continuation_indent.
I was able to make the problem go away by making this change, which
reverts a change you'd apparently made since the old version of indent:

diff -ru /home/postgres/freebsd_indent/indent.c freebsd_indent/indent.c
--- /home/postgres/freebsd_indent/indent.c    2017-06-13 11:53:59.474524563 -0400
+++ freebsd_indent/indent.c    2017-06-13 15:51:23.590319885 -0400
@@ -944,7 +944,7 @@        }        ps.in_or_st = true;    /* this might be a structure or initialization
 * declaration */
 
-        ps.in_decl = ps.decl_on_line = ps.last_token != type_def;
+        ps.in_decl = ps.decl_on_line = true;        if ( /* !ps.in_or_st && */ ps.dec_nest <= 0)
ps.just_saw_decl= 2;        prefix_blankline_requested = 0;
 

This also undoes a tendency of the new version to want to insert blank
lines that weren't there before inside struct declarations, eg

*** a/contrib/btree_gist/btree_macaddr8.c
--- b/contrib/btree_gist/btree_macaddr8.c
*************** typedef struct
*** 12,17 ****
--- 12,18 ---- {     macaddr8    lower;     macaddr8    upper;
+      /* make struct size = sizeof(gbtreekey16) */ } mac8KEY;

While I would not necessarily have quibbled with the addition of those
blank lines, I'm just as happy not to have them forced on us.  I could
not find any places where reverting this change made the results worse,
so I'm unclear on why you made it.
        regards, tom lane



On 2017-06-13 18:22, Tom Lane wrote:
> The Makefile is still BSD-ish of course, but I think
> we'll just agree to disagree there.

For compiling indent under Linux I use bmake(1). I have no problem with
including a Makefile for GNU Make in my repository.

As I understand it, there will be a copy of indent maintained by the
Postgres group - even less of a problem to include whatever you want, it
seems to me.

I think that Postgres will have to fork FreeBSD indent anyway, because
of nitems(), capsicum, __FBSDID(), etc. Now I only aim for the shortest
diff output.

> The only thing I could find to
> quibble about is that on old macOS versions I get
> 
> In file included from indent.c:49:
> indent_globs.h:222:1: warning: "STACKSIZE" redefined
> In file included from /usr/include/machine/param.h:30,
>                  from /usr/include/sys/param.h:104,
>                  from indent.c:42:
> /usr/include/ppc/param.h:53:1: warning: this is the location of the previous definition
> 
> Maybe you could rename that symbol to IND_STACKSIZE or some such?

I just replaced it with integer constants and nitems().

> Also, I am wondering about the test cases under tests/.  I do not
> see anything in the Makefile or elsewhere suggesting how those are
> to be used.  It would sure be nice to have some quick smoke-test
> to check that a build on a new platform is working.

They'd started out like David Holland's tests for his tradcpp(1), with a
similar makefile (again, BSD make). But I was tenaciously asked to use
Kyua (a testing framework that is the standard regression test mechanism
for FreeBSD) instead, so now the makefile's existence and use is a great
secret and the file is not under any source control. Adaption of the
indent test suite to Kyua made the makefile more inelegant, but I'm
attaching it to this email in hope that you can do something useful with
it. I can only guess that you have the option to use Kyua instead, but I
don't know the tool at all.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Вложения
Attached is a diff from git HEAD to what I get with a slightly-patched
copy of Piotr Stefaniak's latest version of FreeBSD indent.  It looks
pretty darn good to me: the changes around function typedefs and
expressions containing things like sizeof() calls are alone worth the
price of admission.  There are some places where it inserts extra
space because it doesn't know that a given symbol is a typedef name,
but as I remarked upthread, that's the fault of our typedef-list
collection technology not indent itself.  There are not enough of those
places to make me feel like we have to have a solution right now.

There are also places where it changes its mind on how much to indent
a comment-to-the-right-of-code.  I have a hack in my indent copy that
reduces the number of such diffs, but doesn't eliminate them entirely.
Removing the hack will cause indent to align such comments to the next
4-space tab stop rather than the next 8-space stop as has been its
behavior in the past.  I think we should accept that change, because it
will allow squeezing a bit more text into such comments than before, but
in the interests of keeping the attached diff readable I've tried to
suppress most of those changes for the moment.

Anyway, it is now time to fish or cut bait.  I don't think we can wait
much longer to decide whether we're going to adopt this new indent
version for PG 10.  I think we should.  The floor is open for votes.

            regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Вложения
On Tue, Jun 13, 2017 at 05:00:31PM -0400, Tom Lane wrote:
> Anyway, it is now time to fish or cut bait.  I don't think we can wait
> much longer to decide whether we're going to adopt this new indent
> version for PG 10.  I think we should.  The floor is open for votes.

Works for me.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +



On 2017-06-13 22:23, Tom Lane wrote:
> I could not find any places where reverting this change made the
> results worse, so I'm unclear on why you made it.

I must admit I'm a bit confused about why it's not fixed yet, but I'll
have to analyze that later this week. But the idea was to convince
indent that the following is not a declaration and therefore it
shouldn't be formatted as such:

typedef void (*voidptr) (int *);

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-06-13 22:23, Tom Lane wrote:
>> I could not find any places where reverting this change made the
>> results worse, so I'm unclear on why you made it.

> I must admit I'm a bit confused about why it's not fixed yet, but I'll
> have to analyze that later this week. But the idea was to convince
> indent that the following is not a declaration and therefore it
> shouldn't be formatted as such:

> typedef void (*voidptr) (int *);

Hm.  But that's just a function pointer typedef, and we like the
formatting we're getting for those from this new version --- with or
without that change.  What do you think needs to be done differently?

I note btw that this is not the first time we've discussed that
particular bit of code in this thread.  I proposed a couple of
different possible changes to it before ...
        regards, tom lane



btw, I was slightly amused to notice that this version still calls itself

$ indent -V
pg_bsd_indent 1.3

Don't know what you plan to do with that program name, but we certainly
need a version number bump so that pgindent can tell that it's got an
up-to-date copy.  1.4?  2.0?
        regards, tom lane



On Wed, Jun 14, 2017 at 10:38:40AM -0400, Tom Lane wrote:
> btw, I was slightly amused to notice that this version still calls itself
> 
> $ indent -V
> pg_bsd_indent 1.3
> 
> Don't know what you plan to do with that program name, but we certainly
> need a version number bump so that pgindent can tell that it's got an
> up-to-date copy.  1.4?  2.0?

For Piotr's reference, we will update src/tools/pgindent/pgindent to
match whatever new version number you use.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +



Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> On 2017-06-13 18:22, Tom Lane wrote:
>> Also, I am wondering about the test cases under tests/.  I do not
>> see anything in the Makefile or elsewhere suggesting how those are
>> to be used.  It would sure be nice to have some quick smoke-test
>> to check that a build on a new platform is working.

> They'd started out like David Holland's tests for his tradcpp(1), with a
> similar makefile (again, BSD make). But I was tenaciously asked to use
> Kyua (a testing framework that is the standard regression test mechanism
> for FreeBSD) instead, so now the makefile's existence and use is a great
> secret and the file is not under any source control. Adaption of the
> indent test suite to Kyua made the makefile more inelegant, but I'm
> attaching it to this email in hope that you can do something useful with
> it. I can only guess that you have the option to use Kyua instead, but I
> don't know the tool at all.

Ah, thanks.  I hacked up a gmake rule for this:

test: $(INDENT)cp $(srcdir)/tests/*.list .for testsrc in $(srcdir)/tests/*.0; do \test=`basename "$$testsrc" .0`;
\./$(INDENT)$$testsrc $$test.out -P$(srcdir)/tests/$$test.pro || echo FAILED >>$$test.out; \diff -u $$testsrc.stdout
$$test.out|| exit 1; \done 

and I'm getting one failure, which I don't understand:

--- ./tests/f_decls.0.stdout    2017-05-21 19:40:38.507303623 -0400
+++ f_decls.out    2017-06-14 13:28:49.212871476 -0400
@@ -1,4 +1,4 @@
-char *
+char           *x(void){    type        identifier;
@@ -13,7 +13,7 @@    return NULL;}
-int *
+int           *y(void){
Does that test case pass for you?
        regards, tom lane



On 2017-06-14 19:31, Tom Lane wrote:
> Does that test case pass for you?

No, I broke it recently. Sorry.

On 2017-06-14 17:05, Bruce Momjian wrote:
> On Wed, Jun 14, 2017 at 10:38:40AM -0400, Tom Lane wrote:
>> btw, I was slightly amused to notice that this version still calls itself
>>
>> $ indent -V
>> pg_bsd_indent 1.3
>>
>> Don't know what you plan to do with that program name, but we certainly
>> need a version number bump so that pgindent can tell that it's got an
>> up-to-date copy.  1.4?  2.0?
> 
> For Piotr's reference, we will update src/tools/pgindent/pgindent to
> match whatever new version number you use.

I would like to go a bit further than that. I see that GNU indent
doesn't recognize -V, but prints its version if you use the option
--version. I wish to implement the same option for FreeBSD indent, but
that would force a change in how pgindent recognizes indent.

As for the version bump, I briefly considered "9.7.0", but 2.0 seems
more appropriate as the 2 would reflect the fundamental changes that
I've done and the .0 would indicate that it's still rough around edges.

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> I would like to go a bit further than that. I see that GNU indent
> doesn't recognize -V, but prints its version if you use the option
> --version. I wish to implement the same option for FreeBSD indent, but
> that would force a change in how pgindent recognizes indent.

Not really a problem, considering we're making far larger changes
than that to the pgindent script anyway.

> As for the version bump, I briefly considered "9.7.0", but 2.0 seems
> more appropriate as the 2 would reflect the fundamental changes that
> I've done and the .0 would indicate that it's still rough around edges.

Yeah, +1 for 2.0.
        regards, tom lane