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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

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.

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

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

[Piotr Stefaniak]

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

No, I broke it recently. Sorry.

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

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

[Tom Lane]

Piotr Stefaniak  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.stdout2017-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)
 {
typeidentifier;
@@ -13,7 +13,7 @@
return NULL;
 }
 
-int *
+int   *
 y(void)
 {
 
Does that test case pass for you?

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

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

[Bruce Momjian]

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


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

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

[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


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

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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

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

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

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

[Bruce Momjian]

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


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

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

[Piotr Stefaniak]

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.
INDENT_OBJDIR=  ${.OBJDIR:H}
INDENT= ${INDENT_OBJDIR}/indent

TESTS+= binary
TESTS+= comments
TESTS+= cppelsecom
TESTS+= declarations
TESTS+= elsecomment
TESTS+= f_decls
TESTS+= float
TESTS+= label
TESTS+= list_head
TESTS+= nsac
TESTS+= offsetof
TESTS+= parens
TESTS+= sac
TESTS+= struct
TESTS+= surplusbad
TESTS+= types_from_file
TESTS+= wchar

all: run-tests .WAIT show-diffs

$(INDENT):
${MAKE} -C ${INDENT_OBJDIR}

.for T in $(TESTS)
run-tests: $(T).diff

$(T).diff: $(T).run $(T).0.stdout $(INDENT)
-diff -u $(T).0.stdout $(T).run > $(T).diff

$(T).run: $(INDENT) $(T).0
$(INDENT) $(T).0 $(T).run -P$(T).pro 2>&1 || echo FAILED >> $(T).run
.endfor

show-diffs:
@echo '*** Test diffs ***'
.for T in $(TESTS)
@cat $(T).diff
.endfor

clean:
.for T in $(TESTS)
rm -f $(T).run $(T).diff $(T).o $(T).plist
.endfor

good:
.for T in $(TESTS)
cp $(T).run $(T).0.stdout
.endfor

.PHONY: all run-tests show-diffs clean good

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

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

[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 
  {
macaddr8lower;
macaddr8upper;
+ 
/* 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


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

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

[Tom Lane]

Piotr Stefaniak  writes:
>> There's also the portability issues: __FBSDID() and bcopy() and
>>  [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,
 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?

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


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

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

[Bruce Momjian]

On Sun, Jun 11, 2017 at 09:14:36PM +, 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  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 +


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

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

[Piotr Stefaniak]

>>> * 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
>  [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.

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

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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

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.
me@t520 /tmp> git clone https://github.com/pstef/freebsd_indent
Cloning into 'freebsd_indent'...
remote: Counting objects: 640, done.
remote: Compressing objects: 100% (55/55), done.
remote: Total 640 (delta 128), reused 151 (delta 116), pack-reused 469
Receiving objects: 100% (640/640), 270.61 KiB | 66.00 KiB/s, done.
Resolving deltas: 100% (409/409), done.
Checking connectivity... done.
me@t520 /tmp> cd freebsd_indent
me@t520 /t/freebsd_indent> bmake CC=clang CFLAGS='-D__FBSDID="static char 
*rcsid=" -g -O0'
clang -D__FBSDID="static char *rcsid=" -g -O0-c indent.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c io.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c lexi.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c parse.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c pr_comment.c
clang -D__FBSDID="static char *rcsid=" -g -O0-c args.c
clang   -o indent  indent.o io.o lexi.o parse.o pr_comment.o args.o 
nroff -man indent.1 > indent.cat1
me@t520 /t/freebsd_indent> cp ~/postgres/freebsd_indent/test.c .
me@t520 /t/freebsd_indent> ./indent -bad -bap -bc -bl -d0 -cdb -nce -nfc1 -di12 
-i4 -l79 -lp -nip -npro -bbb -cli1 
-U/home/me/pgindent-test/git/src/tools/pgindent/typedefs.list < test.c
typedef struct GinBtreeData
{
/* search methods */
BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *);
BlockNumber (*getLeftMostChild) (GinBtree, Page);
bool(*isMoveRight) (GinBtree, Page);
bool(*findItem) (GinBtree, GinBtreeStack *);

/* insert methods */
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
hardclock_device_poll(void)
{
const unsigned (*TABLE_index)[2];

if (stat(pg_data, ) != 0)
{
/*
 * The Linux Standard Base Core Specification 3.1 says this should
 * return '4, program or service status is unknown'
 * https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
 * eneric/iniscrptact.html
 */
exit(is_status_request ? 4 : 1);
}
}
me@t520 /t/freebsd_indent> cat test.c
typedef struct GinBtreeData
{
/* search methods */
BlockNumber (*findChildPage) (GinBtree, GinBtreeStack *);
BlockNumber (*getLeftMostChild) (GinBtree, Page);
bool(*isMoveRight) (GinBtree, Page);
bool(*findItem) (GinBtree, GinBtreeStack *);

/* insert methods */
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
hardclock_device_poll(void)
{
const unsigned (*TABLE_index)[2];
if (stat(pg_data, ) != 0)
{
/*
 * The Linux Standard Base Core Specification 3.1 says this 
should
 * return '4, program or service status is unknown'
 * 
https://refspecs.linuxbase.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-g
 * eneric/iniscrptact.html
 */
exit(is_status_request ? 4 : 1);
}
}
me@t520 /t/freebsd_indent>
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

[Tom Lane]

Piotr Stefaniak  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
.

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

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

[Piotr Stefaniak]

> * 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.


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

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

[Bruce Momjian]

On Sun, May 21, 2017 at 10:12:20AM -0400, Tom Lane wrote:
> Piotr Stefaniak  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  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 +


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

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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

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.



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

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

[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


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

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

[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 bool
 ean2string(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 the header 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 says this 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 terminates the \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;
+   

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

[Tom Lane]

Piotr Stefaniak  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 

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

[Piotr Stefaniak]

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.


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

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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

On 2017-05-17 22:11, Tom Lane wrote:
> Piotr Stefaniak  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.


-   # 

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

[Tom Lane]

Piotr Stefaniak  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


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

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

[Piotr Stefaniak]

On 2017-05-17 20:31, Tom Lane wrote:
> Piotr Stefaniak  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.



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

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

[Andres Freund]

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/*
> 38122928 pg_bsd_indent/Makefile
>107831   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
>101671   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 25240 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 48361 freebsd_indent/Makefile.depend
 7 18 89 freebsd_indent/tests/cppelsecom.0.stdout
 0  1  5 freebsd_indent/tests/nsac.pro
52173   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 20127 freebsd_indent/tests/surplusbad.0.stdout
60177   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 46284 freebsd_indent/tests/f_decls.0.stdout
23 50571 freebsd_indent/tests/parens.0
14 35246 freebsd_indent/tests/list_head.0.stdout
73147900 freebsd_indent/tests/declarations.0.stdout
16 35242 freebsd_indent/tests/list_head.0
21 51214 freebsd_indent/tests/struct.0
 6 20 95 freebsd_indent/tests/float.0
42118555 freebsd_indent/tests/elsecomment.0
13 31134 freebsd_indent/tests/label.0
23 50558 freebsd_indent/tests/parens.0.stdout
27 51286 freebsd_indent/tests/f_decls.0
 9 20125 freebsd_indent/tests/surplusbad.0
 9 31208 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 17102 freebsd_indent/tests/wchar.0.stdout
47118578 freebsd_indent/tests/elsecomment.0.stdout
79143850 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 31209 freebsd_indent/tests/binary.0.stdout
 7 19 96 freebsd_indent/tests/offsetof.0.stdout
14 31207 freebsd_indent/tests/label.0.stdout
 0  1  4 freebsd_indent/tests/comments.pro
23 47215 freebsd_indent/tests/struct.0.stdout
   100818   4720 freebsd_indent/README
51300   2082 freebsd_indent/indent.h
   351   1415  10848 freebsd_indent/args.c
75425   2740 freebsd_indent/indent_codes.h
   654   2577  17238 freebsd_indent/lexi.c
  5366  23567 151406 total


- Andres


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

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

[Tom Lane]

Andres Freund  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/*
38122928 pg_bsd_indent/Makefile
   107831   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
   101671   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.)

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

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

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

[Tom Lane]

Alvaro Herrera  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


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

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

[Andres Freund]

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


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

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

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, 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