Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
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.)
>>> * 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.)
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.)
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, &statbuf) != 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, &statbuf) != 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.)
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.)
> * 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.)
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.)
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.)
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.)
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.)
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; + SH_ELEMENT
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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 found that the difference between
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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.)
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.)
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. - # Remove trailing blank lines -
Re: pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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.)
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.)
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.)
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.)
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.)
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
pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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. 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 -- 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 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
pgindent (was Re: [HACKERS] [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)
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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers