Re: [HACKERS] Preliminary results for proposed new pgindent implementation
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frostwrites: > > * Robert Haas (robertmh...@gmail.com) wrote: > >> On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut > >> wrote: > >>> On 6/16/17 10:51, Tom Lane wrote: > So I'm back to the position that we ought to stick the indent > code under src/tools/ in our main repo. Is anyone really > seriously against that? > > >>> I think it would be better to have it separate. > > >> +1. > > > +1. > > Given the license issues raised downthread, we have no choice in > the short term. So I have a request in to create a separate repo > on git.postgresql.org (whose chain do I need to pull to get that > approved, btw?) u, that would probably be pginfra in some capacity, but I don't recall seeing any notification of such a request. I will follow up with those responsible, #blamemagnus Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Stephen Frostwrites: > * Robert Haas (robertmh...@gmail.com) wrote: >> On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut >> wrote: >>> On 6/16/17 10:51, Tom Lane wrote: So I'm back to the position that we ought to stick the indent code under src/tools/ in our main repo. Is anyone really seriously against that? >>> I think it would be better to have it separate. >> +1. > +1. Given the license issues raised downthread, we have no choice in the short term. So I have a request in to create a separate repo on git.postgresql.org (whose chain do I need to pull to get that approved, btw?) 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: [HACKERS] Preliminary results for proposed new pgindent implementation
* Robert Haas (robertmh...@gmail.com) wrote: > On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentraut >wrote: > > On 6/16/17 10:51, Tom Lane wrote: > >> So I'm back to the position that we ought to stick the indent > >> code under src/tools/ in our main repo. Is anyone really > >> seriously against that? > > > > I think it would be better to have it separate. > > +1. +1. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On Sat, Jun 17, 2017 at 5:41 PM, Peter Eisentrautwrote: > On 6/16/17 10:51, Tom Lane wrote: >> So I'm back to the position that we ought to stick the indent >> code under src/tools/ in our main repo. Is anyone really >> seriously against that? > > I think it would be better to have it separate. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Peter Eisentrautwrites: > On 6/16/17 10:51, Tom Lane wrote: >> So I'm back to the position that we ought to stick the indent >> code under src/tools/ in our main repo. Is anyone really >> seriously against that? > I think it would be better to have it separate. > Other than for reasons of principle and general modularity of the world, > I would like this to be available separately for separate download, > packaging, etc. to it can be applied to extension projects without > having to download and build (a specific version of) PostgreSQL. The > code formatting in extension projects is, um, questionable. In fact, if > it's a better indent period, I would like to package it for the general > public. Well, the direction I'm headed in for addressing the portability issues is to make it depend on the usual PG build environment, notably c.h and libpgport. If we don't want it in-tree, it can be built using PGXS, but it'll still require a PG installation somewhere in order to get built. Making it independent of both FreeBSD and PG is a significantly larger project, and one I don't personally intend to tackle. (And, if someone does tackle that, I don't exactly see why having our own copy in-tree would stop them.) However ... off-list discussion with Piotr indicates that he's unwilling to touch the license text without permission from FreeBSD core and/or legal teams. While the 4-clause license is certainly no impediment to using indent, we don't want any such text in our tree, so that seems like a showstopper, at least until the license question is resolved. Accordingly, I'll proceed with setting up a repo for it on git.postgresql.org. > If the vote is to put it into the tree, I would request not to do it in > PG10. At this point, we should be winding things down and not open up > new areas of activity. I'm confused by this. Are you objecting to switching to the new indent version for v10? 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-17 21:55, Tom Lane wrote: > I spent some time looking into this. I reverted your commits > 198457848ae5c86bec3336a9437dd5aa30f480c2 (Replace err.h functions with > standard C equivalents) and fb10acb040b90bdcbad09defd303363db29257d1 > (Remove inclusion of sys/cdefs.h) locally and tried to build without > those. I wanted to mirror that move, but forgot to not rebase the repository, so I removed those two commits instead of committing their negatives. Sorry about that. > I've successfully worked around the err.h change by adding > cut-down versions of FreeBSD 11's err.h and err.c to the fileset > (see attached). I thought about something like: #ifdef __FreeBSD__ #include #define ERR(...) err(__VA_ARGS__) #define ERRX(...) errx(__VA_ARGS__) #else #include "err.h" #endif and then call ERR() and ERRX() instead of err() and errx(). But that requires C99. And I would have a very hard time convincing anyone that it makes any sense from FreeBSD's perspective, since indent is part of the base system, where is guaranteed to exist. Perhaps it would be best for everyone if indent was moved out of FreeBSD base, so that portability arguments would make more sense. But that would take time and some debate. > However, it's proving impossible to work around having > "#include " as the first live code in the files. I thought > maybe we could provide a dummy cdefs.h file, but that breaks things on > platforms where cdefs.h is a real thing and is relied on by other system > headers --- which includes both Linux and BSD. It seems we would have > to have something like #ifdef HAVE_SYS_CDEFS_H, but that is already a > departure from FreeBSD practice. I was thinking if I could get away with putting those into #ifdef __FreeBSD__ ... #endif. I think that it might be feasible unlike the idea above. I could be wrong. > So what I'm currently thinking is that we have to diverge from the > FreeBSD sources to the extent of removing #include > and the __FBSDID() calls, and instead inserting #include "c.h" to > pick up PG's own portability definitions. The thing that forced me > into the latter is that there seems no way to avoid compiler warnings > if we don't decorate the declarations of err() and errx() with noreturn > and printf-format attributes --- and we need c.h to provide portable > ways of writing those. But there are probably other portability things > that we'll need c.h for, anyway, especially if we want to make it work > on Windows. So I'm thinking this is a small and easily maintainable > difference from the upstream FreeBSD files. That works for me. > When I inserted #include "c.h", I got duplicate-macro-definition warnings > about "true" and "false", so I would ask you to add this: Done. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 6/16/17 10:51, Tom Lane wrote: > So I'm back to the position that we ought to stick the indent > code under src/tools/ in our main repo. Is anyone really > seriously against that? I think it would be better to have it separate. Other than for reasons of principle and general modularity of the world, I would like this to be available separately for separate download, packaging, etc. to it can be applied to extension projects without having to download and build (a specific version of) PostgreSQL. The code formatting in extension projects is, um, questionable. In fact, if it's a better indent period, I would like to package it for the general public. If the vote is to put it into the tree, I would request not to do it in PG10. At this point, we should be winding things down and not open up new areas of activity. There is a chance that if this goes in (or anywhere else), there will be a stream of requests along the lines of: doesn't build on Windows, doesn't build on AIX, doesn't build on PowerPC, doesn't build on this other Windows variant, the tests don't run, the tests don't run on Windows, it doesn't build in vpath, it doesn't work on the buildfarm, and so on. -- Peter Eisentraut http://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
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
I wrote: > Piotr Stefaniakwrites: >> There are also the "portability fixes" and they're the main problem. > Fair enough. I spent some time looking into this. I reverted your commits 198457848ae5c86bec3336a9437dd5aa30f480c2 (Replace err.h functions with standard C equivalents) and fb10acb040b90bdcbad09defd303363db29257d1 (Remove inclusion of sys/cdefs.h) locally and tried to build without those. I've successfully worked around the err.h change by adding cut-down versions of FreeBSD 11's err.h and err.c to the fileset (see attached). However, it's proving impossible to work around having "#include " as the first live code in the files. I thought maybe we could provide a dummy cdefs.h file, but that breaks things on platforms where cdefs.h is a real thing and is relied on by other system headers --- which includes both Linux and BSD. It seems we would have to have something like #ifdef HAVE_SYS_CDEFS_H, but that is already a departure from FreeBSD practice. So what I'm currently thinking is that we have to diverge from the FreeBSD sources to the extent of removing #include and the __FBSDID() calls, and instead inserting #include "c.h" to pick up PG's own portability definitions. The thing that forced me into the latter is that there seems no way to avoid compiler warnings if we don't decorate the declarations of err() and errx() with noreturn and printf-format attributes --- and we need c.h to provide portable ways of writing those. But there are probably other portability things that we'll need c.h for, anyway, especially if we want to make it work on Windows. So I'm thinking this is a small and easily maintainable difference from the upstream FreeBSD files. When I inserted #include "c.h", I got duplicate-macro-definition warnings about "true" and "false", so I would ask you to add this: --- freebsd_indent/indent_globs.h 2017-06-16 11:06:53.329712682 -0400 +++ new/indent_globs.h 2017-06-17 14:45:41.388015754 -0400 @@ -43,8 +43,12 @@ * of code */ +#ifndef false #define false 0 +#endif +#ifndef true #define true 1 +#endif FILE *input; /* the fid for the input file */ Other than that, I think this is a workable compromise on the portability questions. regards, tom lane /*- * Copyright (c) 1993 * The Regents of the University of California. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright *notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright *notice, this list of conditions and the following disclaimer in the *documentation and/or other materials provided with the distribution. * 3. Neither the name of the University nor the names of its contributors *may be used to endorse or promote products derived from this software *without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE REGENTS AND CONTRIBUTORS ``AS IS'' AND * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE * ARE DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. * * @(#)err.h 8.1 (Berkeley) 6/2/93 * $FreeBSD: stable/11/include/err.h 203964 2010-02-16 19:39:50Z imp $ */ #ifndef _ERR_H_ #define _ERR_H_ /* * This is cut down to just the minimum that we need to build indent. */ voiderr(int, const char *, ...) pg_attribute_noreturn() pg_attribute_printf(2, 3); voiderrx(int, const char *, ...) pg_attribute_noreturn() pg_attribute_printf(2, 3); #endif /* !_ERR_H_ */ /*- * Copyright (c) 1993 * The Regents of the University of California. All rights reserved. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright *notice, this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright *notice, this list of conditions and the following disclaimer in the *documentation and/or other materials provided with the distribution. * 3.
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Robert Haaswrites: > On Fri, Jun 16, 2017 at 10:51 AM, Tom Lane wrote: >> So I'm back to the position that we ought to stick the indent >> code under src/tools/ in our main repo. Is anyone really >> seriously against that? > Is it under the same license as everything else? Hm, now that you mention it, interesting point. I was about to answer that it's under the standard BSD license, but looking closer, I see that these files still quote the old 4-clause BSD license text (with the "advertising" clause). We need to get them adjusted to be 3-clause with no advertising. Looking into a FreeBSD source tree locally, I see that FreeBSD has removed the advertising clause from all their files --- sometimes without even remembering to renumber the old clause 4 to clause 3 ;-). So Piotr needs to do likewise to conform with FreeBSD policy. Once he has, it'll be identical text to the other BSD-origin files we have in our tree, eg src/port/getopt.c. Piotr: if you're unclear on the rationale for this, see the bottom of https://www.freebsd.org/copyright/license.html 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, Jun 16, 2017 at 10:51 AM, Tom Lanewrote: > So I'm back to the position that we ought to stick the indent > code under src/tools/ in our main repo. Is anyone really > seriously against that? Is it under the same license as everything else? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Piotr Stefaniakwrites: > On 2017-06-17 00:02, Tom Lane wrote: >> What I'm testing with right now has just four differences from your repo: > There are also the "portability fixes" and they're the main problem. Fair enough. > I've simply removed things like capsicum or __FBSDID() because I thought > it wouldn't be a problem since Postgres will have its own copy of indent > anyway (so that its behavior is not a moving target). I can ifdef-out > them instead of removing entirely, I just didn't think it was important > anymore. We should be able to deal with those via some #define hackery, no? > I expect to be in trouble for replacing err() and errx(), though. Understood. I think we could deal with this by providing err() and errx() in a support file that would be part of our distribution but not yours. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-17 00:02, Tom Lane wrote: > Piotr Stefaniakwrites: >> On 2017-06-16 21:56, Tom Lane wrote: >>> Unless Piotr objects, I propose to add another switch to bsdindent >>> that selects this behavior, and then we can drop entab, removing >>> another impediment to getting pgindent working. > >> I understand the reasoning, but this is a very specific need and I think >> not at all universal for anyone else in the future. One of the bugs >> listed in indent's manpage is that it "has more switches than ls(1)". So >> currently I'm against pushing an option for the above upstream, to the >> FreeBSD repository. > >> Why not add this to the already non-empty list of custom patches? > > Umm ... I thought the idea was to get to the point where the list of > custom patches *is* empty. Except for carrying our own Makefile of > course. I'd be sad if we needed a fork just for this. > > What I'm testing with right now has just four differences from your repo: There are also the "portability fixes" and they're the main problem. I've simply removed things like capsicum or __FBSDID() because I thought it wouldn't be a problem since Postgres will have its own copy of indent anyway (so that its behavior is not a moving target). I can ifdef-out them instead of removing entirely, I just didn't think it was important anymore. I expect to be in trouble for replacing err() and errx(), though. > 1. This workaround for what I believe you agree is a bug: > > - ps.in_decl = ps.decl_on_line = ps.last_token != type_def; > + ps.in_decl = ps.decl_on_line = true; That will need a proper fix... > 2. The long-lines adjustment I just sent you a patch for. That looks very good. > 3. The tab-vs-space difference under discussion here. I can be convinced to make it another option upstream. But I dislike it nevertheless. > 4. A temporary hack affecting the indentation of comments on the same line > (forcing them to a multiple of 8 spaces even though tabsize is 4). I have > every intention of dropping that one later; I just don't want to deal with > comment reindentation at the same time as these other things. Great! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Piotr Stefaniakwrites: > On 2017-06-16 21:56, Tom Lane wrote: >> Unless Piotr objects, I propose to add another switch to bsdindent >> that selects this behavior, and then we can drop entab, removing >> another impediment to getting pgindent working. > I understand the reasoning, but this is a very specific need and I think > not at all universal for anyone else in the future. One of the bugs > listed in indent's manpage is that it "has more switches than ls(1)". So > currently I'm against pushing an option for the above upstream, to the > FreeBSD repository. > Why not add this to the already non-empty list of custom patches? Umm ... I thought the idea was to get to the point where the list of custom patches *is* empty. Except for carrying our own Makefile of course. I'd be sad if we needed a fork just for this. What I'm testing with right now has just four differences from your repo: 1. This workaround for what I believe you agree is a bug: - ps.in_decl = ps.decl_on_line = ps.last_token != type_def; + ps.in_decl = ps.decl_on_line = true; 2. The long-lines adjustment I just sent you a patch for. 3. The tab-vs-space difference under discussion here. 4. A temporary hack affecting the indentation of comments on the same line (forcing them to a multiple of 8 spaces even though tabsize is 4). I have every intention of dropping that one later; I just don't want to deal with comment reindentation at the same time as these other things. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Piotr Stefaniakwrites: > On 2017-06-16 20:11, Tom Lane wrote: >> I assume though that Piotr wants an option to preserve that behavior. >> I'm happy to write up a patch for bsdindent that adds a switch >> controlling this, but is there any rhyme or reason to the way its >> switches are named? > I don't want to preserve the current behavior at all, but I might need > to add an option for choosing one or the other if users of FreeBSD > indent protest. > I don't have a good name for it. The best I can do is -lpl ("-lp long > lines too"). Can I see the patch? Here's a patch. An alternative switch name might be -lpa ("-lp always") but I'm not set on that. regards, tom lane diff -pudr indent-curr/args.c indent-lpl/args.c --- indent-curr/args.c 2017-06-16 11:06:53.329712682 -0400 +++ indent-lpl/args.c 2017-06-16 17:43:56.473230024 -0400 @@ -125,6 +125,7 @@ struct pro { {"i", PRO_INT, 8, 0, _size}, {"lc", PRO_INT, 0, 0, _comment_max_col}, {"ldi", PRO_INT, -1, 0, _decl_indent}, +{"lpl", PRO_BOOL, false, ON, _to_parens_always}, {"lp", PRO_BOOL, true, ON, _to_parens}, {"l", PRO_INT, 78, 0, _col}, {"nbacc", PRO_BOOL, false, OFF, _around_conditional_compilation}, @@ -143,6 +144,7 @@ struct pro { {"nfc1", PRO_BOOL, true, OFF, _col1_comments}, {"nfcb", PRO_BOOL, true, OFF, _block_comments}, {"nip", PRO_BOOL, true, OFF, _parameters}, +{"nlpl", PRO_BOOL, false, OFF, _to_parens_always}, {"nlp", PRO_BOOL, true, OFF, _to_parens}, {"npcs", PRO_BOOL, false, OFF, _calls_space}, {"npro", PRO_SPECIAL, 0, IGN, 0}, diff -pudr indent-curr/indent.1 indent-lpl/indent.1 --- indent-curr/indent.1 2017-06-16 17:18:05.697722416 -0400 +++ indent-lpl/indent.1 2017-06-16 17:26:53.203823690 -0400 @@ -74,6 +74,7 @@ .Op Fl \ Ns Ar n .Op Fl \ Ns Ar n .Op Fl \ | Fl nlp +.Op Fl \ | Fl nlpl .Op Fl npro .Op Fl P Ns Ar file .Op Fl pcs | Fl npcs @@ -388,6 +389,19 @@ p1\ =\ first_procedure(second_procedure( \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ third_procedure(p4, \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ p5)); .Ed +.It Fl \ , nlpl +With +.Fl \ , +code surrounded by parentheses in continuation lines is lined up even if it +would extend past the right margin. +With +.Fl \ +(the default), such a line that would extend past the right margin is moved +left to keep it within the margin, if that does not require placing it to +the left of the prevailing indentation level. +These switches have no effect if +.Fl nlp +is selected. .It Fl npro Causes the profile files, .Sq Pa ./.indent.pro diff -pudr indent-curr/indent.c indent-lpl/indent.c --- indent-curr/indent.c 2017-06-13 11:53:59.474524563 -0400 +++ indent-lpl/indent.c 2017-06-16 17:29:11.924267996 -0400 @@ -160,6 +160,7 @@ main(int argc, char **argv) #ifdef undef max_col = 78; /* -l78 */ lineup_to_parens = 1; /* -lp */ +lineup_to_parens_always = 0; /* -nlpl */ ps.ljust_decl = 0; /* -ndj */ ps.com_ind = 33; /* -c33 */ star_comment_cont = 1; /* -sc */ diff -pudr indent-curr/indent_globs.h indent-lpl/indent_globs.h --- indent-curr/indent_globs.h 2017-06-16 11:06:53.329712682 -0400 +++ indent-lpl/indent_globs.h 2017-06-16 17:30:14.664826384 -0400 @@ -185,6 +185,8 @@ int continuation_indent;/* set t * code and continuation lines */ int lineup_to_parens; /* if true, continued code within parens will * be lined up to the open paren */ +int lineup_to_parens_always; /* if true, do not attempt to keep + * lined-up code within the margin */ int Bill_Shannon; /* true iff a blank should always be inserted * after sizeof */ int blanklines_after_declarations_at_proctop; /* This is vaguely diff -pudr indent-curr/io.c indent-lpl/io.c --- indent-curr/io.c 2017-06-13 11:53:59.475524587 -0400 +++ indent-lpl/io.c 2017-06-16 17:31:11.233230896 -0400 @@ -221,6 +221,8 @@ compute_code_target(void) if (!lineup_to_parens) target_col += continuation_indent * (2 * continuation_indent == ps.ind_size ? 1 : ps.paren_level); + else if (lineup_to_parens_always) + target_col = paren_target; else { int w; int t = paren_target; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-16 20:11, Tom Lane wrote: > Andres Freundwrites: >> On 2017-06-16 13:44:30 -0400, Bruce Momjian wrote: >>> Yes, it is all about <80 column output. The current pgindent does >>> everything possible to accomplish that --- the question is whether we >>> want uglier code to do it. > >> For me personally the misindentation is way uglier than a too long line. > > I assume though that Piotr wants an option to preserve that behavior. > I'm happy to write up a patch for bsdindent that adds a switch > controlling this, but is there any rhyme or reason to the way its > switches are named? I don't want to preserve the current behavior at all, but I might need to add an option for choosing one or the other if users of FreeBSD indent protest. I don't have a good name for it. The best I can do is -lpl ("-lp long lines too"). Can I see the patch? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-16 21:56, Tom Lane wrote: > One other thing I'd like to do while we're changing this stuff is > to get rid of the need for entab/detab. Right now, after doing > all the other work, my copy of pgindent is running the code through > detab and then entab so as to match the old decisions about how to > represent whitespace (ie, as spaces or tabs). This is grotty as > can be. I managed to tweak bsdindent so that its output matches > what entab would do, by dint of the attached patch, which implements > the rule "use a space instead of a tab if the tab would only move > one column and we don't need another tab after it". (I think entab > is being weird with the second half of that rule, but if I remove it, > I get circa a thousand lines of invisible whitespace changes; probably > better not to deal with those. With no patch at all, just letting > bsdindent do what it does now, there's circa ten thousand changed lines.) > > Unless Piotr objects, I propose to add another switch to bsdindent > that selects this behavior, and then we can drop entab, removing > another impediment to getting pgindent working. I understand the reasoning, but this is a very specific need and I think not at all universal for anyone else in the future. One of the bugs listed in indent's manpage is that it "has more switches than ls(1)". So currently I'm against pushing an option for the above upstream, to the FreeBSD repository. Why not add this to the already non-empty list of custom patches? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, Jun 16, 2017 at 03:56:47PM -0400, Tom Lane wrote: > can be. I managed to tweak bsdindent so that its output matches > what entab would do, by dint of the attached patch, which implements > the rule "use a space instead of a tab if the tab would only move > one column and we don't need another tab after it". (I think entab > is being weird with the second half of that rule, but if I remove it, > I get circa a thousand lines of invisible whitespace changes; probably > better not to deal with those. With no patch at all, just letting > bsdindent do what it does now, there's circa ten thousand changed lines.) Yeah, entab was designed to do that, via this C comment: /* * Is the next character going to be a tab? We do tab * replacement in the current spot if the next char is * going to be a tab and ignore min_spaces. */ -- Bruce Momjianhttp://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: [HACKERS] Preliminary results for proposed new pgindent implementation
One other thing I'd like to do while we're changing this stuff is to get rid of the need for entab/detab. Right now, after doing all the other work, my copy of pgindent is running the code through detab and then entab so as to match the old decisions about how to represent whitespace (ie, as spaces or tabs). This is grotty as can be. I managed to tweak bsdindent so that its output matches what entab would do, by dint of the attached patch, which implements the rule "use a space instead of a tab if the tab would only move one column and we don't need another tab after it". (I think entab is being weird with the second half of that rule, but if I remove it, I get circa a thousand lines of invisible whitespace changes; probably better not to deal with those. With no patch at all, just letting bsdindent do what it does now, there's circa ten thousand changed lines.) Unless Piotr objects, I propose to add another switch to bsdindent that selects this behavior, and then we can drop entab, removing another impediment to getting pgindent working. regards, tom lane diff -ur /home/postgres/freebsd_indent/indent.c ./indent.c --- /home/postgres/freebsd_indent/indent.c 2017-06-13 11:53:59.474524563 -0400 +++ ./indent.c 2017-06-16 15:41:53.515416614 -0400 @@ -1275,7 +1275,7 @@ CHECK_SIZE_CODE(cur_dec_ind / tabsize); while ((tpos = tabsize * (1 + pos / tabsize)) <= cur_dec_ind) { - *e_code++ = '\t'; + *e_code++ = (tpos > pos + 1 || cur_dec_ind >= tpos + tabsize) ? '\t' : ' '; pos = tpos; } } diff -ur /home/postgres/freebsd_indent/io.c ./io.c --- /home/postgres/freebsd_indent/io.c 2017-06-13 11:53:59.475524587 -0400 +++ ./io.c 2017-06-16 15:42:47.686762023 -0400 @@ -399,7 +399,7 @@ int tcur; while ((tcur = tabsize * (1 + (curr - 1) / tabsize) + 1) <= target) { - putc('\t', output); + putc((tcur > curr + 1 || target >= tcur + tabsize) ? '\t' : ' ', output); curr = tcur; } } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, Jun 16, 2017 at 11:54:06AM -0700, Andres Freund wrote: > On 2017-06-16 14:42:38 -0400, Bruce Momjian wrote: > > On Fri, Jun 16, 2017 at 02:23:00PM -0400, Tom Lane wrote: > > > Well, that's something we need to discuss. I originally argued for > > > back-patching the new rules, whatever they are (ie, run the new > > > pgindent on the back branches whenever we've agreed that the dust > > > has settled). But I'm starting to realize that that's likely to > > > be horrid for anyone who's carrying out-of-tree patches, as I know > > > a lot of packagers do for instance. We have to trade off our own > > > inconvenience in making back-patches against inconvenience to > > > people who are maintaining private patchsets. > > > > Can't they sync up to just before our pgindent commit and run pgindent > > on their own code base? > > That doesn't really help that much if you have a series of patches that > you want to keep independent, e.g. because you might want to submit to > postgres. And you'll also get a bunch of annoying to resolve merge > conflicts, even if they're easier to resolve with that methodology. I think we have to ask how much we want to make things easier for people with modified but continually-updated Postgres trees vs. our community-tree developers. -- Bruce Momjianhttp://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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-16 14:42:38 -0400, Bruce Momjian wrote: > On Fri, Jun 16, 2017 at 02:23:00PM -0400, Tom Lane wrote: > > Well, that's something we need to discuss. I originally argued for > > back-patching the new rules, whatever they are (ie, run the new > > pgindent on the back branches whenever we've agreed that the dust > > has settled). But I'm starting to realize that that's likely to > > be horrid for anyone who's carrying out-of-tree patches, as I know > > a lot of packagers do for instance. We have to trade off our own > > inconvenience in making back-patches against inconvenience to > > people who are maintaining private patchsets. > > Can't they sync up to just before our pgindent commit and run pgindent > on their own code base? That doesn't really help that much if you have a series of patches that you want to keep independent, e.g. because you might want to submit to postgres. And you'll also get a bunch of annoying to resolve merge conflicts, even if they're easier to resolve with that methodology. - 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, Jun 16, 2017 at 02:23:00PM -0400, Tom Lane wrote: > Well, that's something we need to discuss. I originally argued for > back-patching the new rules, whatever they are (ie, run the new > pgindent on the back branches whenever we've agreed that the dust > has settled). But I'm starting to realize that that's likely to > be horrid for anyone who's carrying out-of-tree patches, as I know > a lot of packagers do for instance. We have to trade off our own > inconvenience in making back-patches against inconvenience to > people who are maintaining private patchsets. Can't they sync up to just before our pgindent commit and run pgindent on their own code base? -- Bruce Momjianhttp://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: [HACKERS] Preliminary results for proposed new pgindent implementation
Andres Freundwrites: > On 2017-06-16 13:34:01 -0400, Tom Lane wrote: >> I do intend to apply the diffs to HEAD in multiple steps, just to >> make them more reviewable. But I think we should probably absorb >> all the changes we want into v10, not leave some for later cycles. > Btw, how much are you planning to backpatch these? Well, that's something we need to discuss. I originally argued for back-patching the new rules, whatever they are (ie, run the new pgindent on the back branches whenever we've agreed that the dust has settled). But I'm starting to realize that that's likely to be horrid for anyone who's carrying out-of-tree patches, as I know a lot of packagers do for instance. We have to trade off our own inconvenience in making back-patches against inconvenience to people who are maintaining private patchsets. One idea that occurs to me after a few minutes' thought is to announce that we will reindent the back branches, but not till around the time of v10 final release. Once v10 is out, anybody who's carrying a private patchset will be needing to think about rebasing it on top of reindented code anyway, so dealing with that in the back branches at the same time might be a bit less work. Or we could leave the back branches alone and anticipate five years worth of pain in back-patching. I don't find that very appetizing personally, but it might be the easiest sell to the majority of the community, since very few of us do back-patching work on a regular basis. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Andres Freundwrites: > On 2017-06-16 13:44:30 -0400, Bruce Momjian wrote: >> Yes, it is all about <80 column output. The current pgindent does >> everything possible to accomplish that --- the question is whether we >> want uglier code to do it. > For me personally the misindentation is way uglier than a too long line. I'm coming around to that opinion too. We have many source lines that are a bit too long, or a lot too long if someone decided they didn't want to split an error message across lines. pgindent "fixes" that in some places but not others (if it would have to go left of the prevailing statement indent, it gives up and indents to the paren level anyway). On balance it's just weird. Better to indent normally and let the programmer decide if she wants to break the lines differently to keep them from wrapping. I assume though that Piotr wants an option to preserve that behavior. I'm happy to write up a patch for bsdindent that adds a switch controlling this, but is there any rhyme or reason to the way its switches are named? 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-16 13:34:01 -0400, Tom Lane wrote: > > I could live with both of these proposed > > changes, the selection of the changes you posted looks like it could be > > improved by code changes, but that's obviously a large amount of work. > > In the end, the only thing that fixes this sort of stuff is to be more > rigid about making the code fit into 80 columns to begin with. I get > the impression though that a lot of people work in editor windows that > are wider than that, so the code looks fine to them when it slops over > a bit. That, but maybe also that it's often slightly too long line vs. weird multiline mess. A good number of things pgindent indents weirdly can be prevented by just not adding a linebreak, which isn't a great fix... > > At this point however I wonder whether just moving to the new tool on > > its own wouldn't be a big enough change - we could just delay that > > decision until we've got the rest done at least. > > I'm torn between that approach and "let's just have one big flag day > and get it over with". I don't have a strong opinion on this. > I think having the rules incrementally changing from one release to > the next will be a huge headache. Yea, I was more thinking of getting the new indent in, and then making the followup decisions a few days after. > I do intend to apply the diffs to HEAD in multiple steps, just to > make them more reviewable. But I think we should probably absorb > all the changes we want into v10, not leave some for later cycles. Btw, how much are you planning to backpatch these? Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-06-16 13:44:30 -0400, Bruce Momjian wrote: > On Fri, Jun 16, 2017 at 01:34:01PM -0400, Tom Lane wrote: > > > I could live with both of these proposed > > > changes, the selection of the changes you posted looks like it could be > > > improved by code changes, but that's obviously a large amount of work. > > > > In the end, the only thing that fixes this sort of stuff is to be more > > rigid about making the code fit into 80 columns to begin with. I get > > the impression though that a lot of people work in editor windows that > > are wider than that, so the code looks fine to them when it slops over > > a bit. > > Yes, it is all about <80 column output. The current pgindent does > everything possible to accomplish that --- the question is whether we > want uglier code to do it. For me personally the misindentation is way uglier than a too long line. I think a number of those long-lines are there because pgindent sometimes re-indents lines that are continuations of previous ones pretty far, making it hard to reduce indentation. - 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, Jun 16, 2017 at 01:34:01PM -0400, Tom Lane wrote: > > I could live with both of these proposed > > changes, the selection of the changes you posted looks like it could be > > improved by code changes, but that's obviously a large amount of work. > > In the end, the only thing that fixes this sort of stuff is to be more > rigid about making the code fit into 80 columns to begin with. I get > the impression though that a lot of people work in editor windows that > are wider than that, so the code looks fine to them when it slops over > a bit. Yes, it is all about <80 column output. The current pgindent does everything possible to accomplish that --- the question is whether we want uglier code to do it. -- Bruce Momjianhttp://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: [HACKERS] Preliminary results for proposed new pgindent implementation
Andres Freundwrites: > I think the current logic is pretty horrible, primarily because it's so > hard to get to manually. Yes, I think that's really the big argument against it: no editor on the face of the planet will indent code that way to start with. > I could live with both of these proposed > changes, the selection of the changes you posted looks like it could be > improved by code changes, but that's obviously a large amount of work. In the end, the only thing that fixes this sort of stuff is to be more rigid about making the code fit into 80 columns to begin with. I get the impression though that a lot of people work in editor windows that are wider than that, so the code looks fine to them when it slops over a bit. > At this point however I wonder whether just moving to the new tool on > its own wouldn't be a big enough change - we could just delay that > decision until we've got the rest done at least. I'm torn between that approach and "let's just have one big flag day and get it over with". I think having the rules incrementally changing from one release to the next will be a huge headache. I do intend to apply the diffs to HEAD in multiple steps, just to make them more reviewable. But I think we should probably absorb all the changes we want into v10, not leave some for later cycles. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Hi, On 2017-06-16 13:10:50 -0400, Tom Lane wrote: > I experimented with disabling that logic and just always aligning > to the paren indentation. That fixes the weird cases with continued > string literals, but it also makes for a heck of a lot of other changes. > The full diff is too big to post here, but I've attached a selection > of diff hunks to give you an idea. I'm not really sure if I like this > better than pgindent's traditional behavior --- but it's arguably less > confusing. > > An intermediate position that we could consider is to disable the back-off > logic only when the line starts with a string literal. I haven't actually > coded this but it looks like it would be easy, if grotty. I think the current logic is pretty horrible, primarily because it's so hard to get to manually. I could live with both of these proposed changes, the selection of the changes you posted looks like it could be improved by code changes, but that's obviously a large amount of work. The heuristic also seems to make sense. At this point however I wonder whether just moving to the new tool on its own wouldn't be a big enough change - we could just delay that decision until we've got the rest done at least. - 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: [HACKERS] Preliminary results for proposed new pgindent implementation
There was some discussion upthread about how we'd like pgindent not to do weird things with string literals that wrap around the end of the line a little bit. I looked into that and found that it's actually a generic behavior for any line that's within parentheses: normally, such a line will get lined up with the parens, like this: foobar(baz, baz2, baz3, ... but if the line would wrap when indented that much, and backing off lets it not wrap, then it backs off. I experimented with disabling that logic and just always aligning to the paren indentation. That fixes the weird cases with continued string literals, but it also makes for a heck of a lot of other changes. The full diff is too big to post here, but I've attached a selection of diff hunks to give you an idea. I'm not really sure if I like this better than pgindent's traditional behavior --- but it's arguably less confusing. An intermediate position that we could consider is to disable the back-off logic only when the line starts with a string literal. I haven't actually coded this but it looks like it would be easy, if grotty. Or we could leave it alone. Thoughts? regards, tom lane diff -ur pgsql/contrib/amcheck/verify_nbtree.c pgsql-dup/contrib/amcheck/verify_nbtree.c --- pgsql/contrib/amcheck/verify_nbtree.c 2017-06-16 12:31:36.900504080 -0400 +++ pgsql-dup/contrib/amcheck/verify_nbtree.c 2017-06-16 12:35:21.042052427 -0400 @@ -240,8 +240,8 @@ ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot access temporary tables of other sessions"), - errdetail("Index \"%s\" is associated with temporary relation.", - RelationGetRelationName(rel; + errdetail("Index \"%s\" is associated with temporary relation.", + RelationGetRelationName(rel; if (!IndexIsValid(rel->rd_index)) ereport(ERROR, @@ -411,12 +411,12 @@ ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("block %u fell off the end of index \"%s\"", - current, RelationGetRelationName(state->rel; +current, RelationGetRelationName(state->rel; else ereport(DEBUG1, (errcode(ERRCODE_NO_DATA), errmsg("block %u of index \"%s\" ignored", - current, RelationGetRelationName(state->rel; +current, RelationGetRelationName(state->rel; goto nextpage; } else if (nextleveldown.leftmost == InvalidBlockNumber) @@ -433,14 +433,14 @@ if (!P_LEFTMOST(opaque)) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), - errmsg("block %u is not leftmost in index \"%s\"", - current, RelationGetRelationName(state->rel; + errmsg("block %u is not leftmost in index \"%s\"", + current, RelationGetRelationName(state->rel; if (level.istruerootlevel && !P_ISROOT(opaque)) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), - errmsg("block %u is not true root in index \"%s\"", - current, RelationGetRelationName(state->rel; + errmsg("block %u is not true root in index \"%s\"", + current, RelationGetRelationName(state->rel; } /* @@ -488,7 +488,7 @@ errmsg("left link/right link pair in index \"%s\" not in agreement", RelationGetRelationName(state->rel)), errdetail_internal("Block=%u left block=%u left link from block=%u.", - current, leftcurrent, opaque->btpo_prev))); + current, leftcurrent, opaque->btpo_prev))); /* Check level, which must be valid for non-ignorable page */ if (level.level != opaque->btpo.level) @@ -497,7 +497,7 @@ errmsg("leftmost down link for level points to block in index \"%s\" whose level is not one level down", RelationGetRelationName(state->rel)), errdetail_internal("Block pointed to=%u expected level=%u level in pointed to block=%u.", - current, level.level, opaque->btpo.level))); + current, level.level, opaque->btpo.level))); /* Verify invariants for page -- all important checks occur here */ bt_target_page_check(state); @@ -508,8 +508,8 @@ if (current == leftcurrent || current == opaque->btpo_prev) ereport(ERROR, (errcode(ERRCODE_INDEX_CORRUPTED), - errmsg("circular link chain found in block %u of index \"%s\"", - current, RelationGetRelationName(state->rel; + errmsg("circular link chain found in block %u of index \"%s\"", + current, RelationGetRelationName(state->rel; leftcurrent = current; current = opaque->btpo_next; @@ -665,17 +665,17 @@ (errcode(ERRCODE_INDEX_CORRUPTED), errmsg("item order invariant violated for index \"%s\"", RelationGetRelationName(state->rel)), - errdetail_internal("Lower index tid=%s (points to %s tid=%s) " - "higher index tid=%s (points to %s tid=%s) " - "page lsn=%X/%X.", - itid, - P_ISLEAF(topaque) ? "heap"
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Peter Eisentrautwrites: > On 5/19/17 13:31, Alvaro Herrera wrote: >> I favor having indent in a separate repository in our Git server, for >> these reasons > I am also in favor of that. >> 0. it's under our control (so we can change rules as we see fit) >> 1. we can have Piotr as a committer there >> 2. we can use the same pgindent version for all Pg branches > 3. We can use pgindent for external code. Now that we've about reached the point of actually making the change, we need to come to a resolution on where we're keeping the new indent code. I thought that Alvaro's point 1 above (we can give Piotr a commit bit) was the only really compelling argument for putting it into a separate repo rather than into our main tree. In other aspects that's a loser --- in particular, it would be hard to have different indent versions for different PG branches, if we chose to run things that way. However, I gather from Piotr's recent remarks[1] that he's not actually excited about doing continuing maintenance on indent, so that advantage now seems illusory. In any case we'd need to keep such a repo pretty well locked down: if it's changing, and different developers pull from it at different times, then we're going to have people working with different indent behaviors, which will make nobody happy. So I'm back to the position that we ought to stick the indent code under src/tools/ in our main repo. Is anyone really seriously against that? regards, tom lane [1] https://www.postgresql.org/message-id/VI1PR03MB119959F4B65F000CA7CD9F6BF2CC0%40VI1PR03MB1199.eurprd03.prod.outlook.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Peter Eisentrautwrites: > On 5/19/17 11:22, Tom Lane wrote: >> I certainly would rather that our version matched something that's under >> active maintenance someplace. But it seems like there are two good >> arguments for having a copy in our tree: > Is pgindent going to be indented by pgindent? If we were going to keep it in our tree, I'd plan to add an exclusion rule to keep pgindent from touching it, as we already have for assorted other files that are copied from external projects. However, it seems like "keep it in a separate repo" is winning, so it's moot. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 5/19/17 11:22, Tom Lane wrote: > I certainly would rather that our version matched something that's under > active maintenance someplace. But it seems like there are two good > arguments for having a copy in our tree: Is pgindent going to be indented by pgindent? -- Peter Eisentraut http://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
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 5/19/17 13:31, Alvaro Herrera wrote: > I favor having indent in a separate repository in our Git server, for > these reasons I am also in favor of that. > 0. it's under our control (so we can change rules as we see fit) > 1. we can have Piotr as a committer there > 2. we can use the same pgindent version for all Pg branches 3. We can use pgindent for external code. -- Peter Eisentraut http://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
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
I wrote: > What I was just looking at is the possibility of absorbing struct > tags ("xllist" in the above) as if they were typedef names. In > at least 95% of our usages, if a struct has a tag then the tag is > also the struct's typedef name. The reason this is interesting > is that it looks like (on at least Linux and macOS) the debug info > captures struct tags even when it misses the corresponding typedef. > We could certainly create a coding rule that struct tags *must* > match struct typedef names for our own code, but I'm not sure what > violations of that convention might appear in system headers. I did an experiment with seeing what would happen to the typedef list if we included struct tags. On my Linux box, that adds about 10% more names (3343 instead of 3028). A lot of them would be good to have, but there are a lot of others that maybe not so much. See attached diff output. I hesitate to suggest any rule as grotty as "take struct tags only if they begin with an upper-case letter", but that would actually work really well, looks like. regards, tom lane --- typedefs.std 2017-05-19 14:41:15.357406399 -0400 +++ typedefs.log 2017-05-19 14:46:11.978739384 -0400 @@ -39,17 +39,24 @@ AggSplit AggState AggStatePerAgg +AggStatePerAggData AggStatePerGroup +AggStatePerGroupData AggStatePerHash +AggStatePerHashData AggStatePerPhase +AggStatePerPhaseData AggStatePerTrans +AggStatePerTransData AggStrategy Aggref AggrefExprState AlenState Alias AllocBlock +AllocBlockData AllocChunk +AllocChunkData AllocPointer AllocSet AllocSetContext @@ -118,6 +125,7 @@ ArrayExprIterState ArrayIOData ArrayIterator +ArrayIteratorData ArrayMapState ArrayMetaState ArrayParseState @@ -153,6 +161,7 @@ BITVECP BMS_Comparison BMS_Membership +BND BN_CTX BOX BTArrayKeyInfo @@ -167,6 +176,7 @@ BTPageStat BTPageState BTParallelScanDesc +BTParallelScanDescData BTScanOpaque BTScanOpaqueData BTScanPos @@ -251,6 +261,7 @@ BufFile Buffer BufferAccessStrategy +BufferAccessStrategyData BufferAccessStrategyType BufferCachePagesContext BufferCachePagesRec @@ -263,6 +274,7 @@ BuildAccumulator BuiltinScript BulkInsertState +BulkInsertStateData CACHESIGN CAC_state CEOUC_WAIT_MODE @@ -295,6 +307,7 @@ CheckpointStatsData CheckpointerRequest CheckpointerShmemStruct +CheckpointerSlotMapping Chromosome City CkptSortItem @@ -351,12 +364,14 @@ CompressorState ConditionVariable ConditionalStack +ConditionalStackData ConfigData ConfigVariable ConnCacheEntry ConnCacheKey ConnStatusType ConnType +ConnectionOption ConnectionStateEnum ConsiderSplitContext Const @@ -424,6 +439,7 @@ CustomExecMethods CustomOutPtrType CustomPath +CustomPathMethods CustomScan CustomScanMethods CustomScanState @@ -453,6 +469,7 @@ DeclareCursorStmt DecodedBkpBlock DecodingOutputState +DecomprData DefElem DefElemAction DefaultACLInfo @@ -484,6 +501,7 @@ DomainIOData DropBehavior DropOwnedStmt +DropRelationCallbackState DropReplicationSlotCmd DropRoleStmt DropStmt @@ -499,6 +517,10 @@ DumpableObjectType DynamicFileList DynamicZoneAbbrev +ECPGgeneric_varchar +ECPGstruct_member +ECPGtype +ECPGtype_information_cache EC_KEY EDGE ENGINE @@ -517,6 +539,7 @@ EditableObjectType ElementsState EnableTimeoutParams +EncStat EndBlobPtrType EndBlobsPtrType EndDataPtrType @@ -607,6 +630,7 @@ FieldStore File FileFdwExecutionState +FileFdwOption FileFdwPlanState FileName FileNameMap @@ -828,7 +852,9 @@ GinPostingList GinQualCounts GinScanEntry +GinScanEntryData GinScanKey +GinScanKeyData GinScanOpaque GinScanOpaqueData GinState @@ -844,6 +870,7 @@ GistSplitUnion GistSplitVector GlobalTransaction +GlobalTransactionData GrantObjectType GrantRoleStmt GrantStmt @@ -900,8 +927,11 @@ HashJoin HashJoinState HashJoinTable +HashJoinTableData HashJoinTuple +HashJoinTupleData HashMemoryChunk +HashMemoryChunkData HashMetaPage HashMetaPageData HashPageOpaque @@ -920,6 +950,7 @@ HeadlineParsedText HeadlineWordEntry HeapScanDesc +HeapScanDescData HeapTuple HeapTupleData HeapTupleFields @@ -967,6 +998,7 @@ IndexRuntimeKeyInfo IndexScan IndexScanDesc +IndexScanDescData IndexScanState IndexStateFlagsAction IndexStmt @@ -1008,6 +1040,7 @@ IterateJsonStringValuesState JEntry JHashState +JhashState Join JoinCostWorkspace JoinExpr @@ -1132,6 +1165,7 @@ LogicalTapeSet MAGIC MBuf +MDCBufData MJEvalResult MVDependencies MVDependency @@ -1152,6 +1186,7 @@ MergeAppendState MergeJoin MergeJoinClause +MergeJoinClauseData MergeJoinState MergePath MergeScanSelCache @@ -1211,10 +1246,14 @@ NullTestType Numeric NumericAggState +NumericData NumericDigit +NumericLong +NumericShort NumericSortSupport NumericSumAccum NumericVar +ONEXIT OP OSAPerGroupState OSAPerQueryState @@ -1241,6 +1280,7 @@ OidOptions OkeysState OldSerXidControl +OldSerXidControlData OldSnapshotControlData OldToNewMapping OldToNewMappingData @@ -1303,6 +1343,7 @@
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Tom Lane wrote: > Robert Haaswrites: > > Yeah, but those advantages could also be gained by putting the > > pgindent tree on git.postgresql.org in a separate repository. Having > > it in the same repository as the actual PostgreSQL code is not > > required nor, in my opinion, particularly desirable. > > It adds an extra step to what a developer has to do to get pgindent > up and running, so it doesn't seem to me like it's helping the goal > of reducing the setup overhead. I favor having indent in a separate repository in our Git server, for these reasons 0. it's under our control (so we can change rules as we see fit) 1. we can have Piotr as a committer there 2. we can use the same pgindent version for all Pg branches I'm thinking that whenever we change the indent rules, we would re-indent supported back-branches, just as Tom's proposing we'd do now (which I endorse). We wouldn't change the rules often, but say if we leave some typedef wonky behavior alone for now, and somebody happens to fix it in the future, then the fix would apply not only to the current tree at the time but also to all older trees, which makes sense. Now, there is a process that can be followed to update a *patch* from an "pre-indent upstream PG" to a "post-indent upstream PG", to avoid manual work in rebasing the patch past pgindent. This can easily be used on branches that can be rebased, also (since they are essentially just a collection of patches). One problem that remains is that this doesn't apply easily to branches that get merged without rebase. I *think* it should be possible to come up with a process that creates a merge commit using pgindent, but I haven't tried. -- Á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
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Andres Freundwrites: > On 2017-05-19 12:21:52 -0400, Robert Haas wrote: >> Yeah, but those advantages could also be gained by putting the >> pgindent tree on git.postgresql.org in a separate repository. Having >> it in the same repository as the actual PostgreSQL code is not >> required nor, in my opinion, particularly desirable. > I'm of the contrary opinion. A lot of the regular churn due to pgindent > right now is because it's inconvenient to run. Having to clone a > separate repository, compile that project, put it into PATH (fun if > there's multiple versions), run pgindent, discover typedefs.list is out > of date, update, run, ... is pretty much a guarantee that'll continue. > If we had a make indent that computed local typedefs list, *added* new > but not removed old ones, we could get much closer to just always being > properly indented. I hadn't really thought of automating it to that extent, but yeah, that seems like an interesting prospect. > The cost of putting it somewhere blow src/tools/pgindent seems fairly > minor. I think the main cost would be bloating distribution tarballs. Although we're talking about adding ~50K to tarballs that are already pushing 20MB, so realistically who's going to notice? If you want to cut the tarball size, let's reopen the discussion about keeping release notes since the dawn of time. Also, having the support in distributed tarballs is not all bad, because it would allow someone working from a tarball rather than a git pull to have pgindent support. Dunno if there are any such someones anymore, but maybe they're out there. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Heikki Linnakangaswrites: > On 05/19/2017 06:48 PM, Tom Lane wrote: >> That's going to catch a lot of things that are just variables, though. >> It might be all right as long as there was manual filtering after it. > At a quick glance, there are only a couple of them. This two cases > caught my eye. In twophase.c: > static struct xllist > { > ... > } records; > IMHO it would actually be an improvement if there was a space rather > than a tab there. Agreed, but if "records" were considered a typedef name, that would likely screw up the formatting of code referencing it. Maybe less badly with this version of indent than our old one, not sure. What I was just looking at is the possibility of absorbing struct tags ("xllist" in the above) as if they were typedef names. In at least 95% of our usages, if a struct has a tag then the tag is also the struct's typedef name. The reason this is interesting is that it looks like (on at least Linux and macOS) the debug info captures struct tags even when it misses the corresponding typedef. We could certainly create a coding rule that struct tags *must* match struct typedef names for our own code, but I'm not sure what violations of that convention might appear in system headers. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Robert Haaswrites: > On Fri, May 19, 2017 at 11:22 AM, Tom Lane wrote: >> I certainly would rather that our version matched something that's under >> active maintenance someplace. But it seems like there are two good >> arguments for having a copy in our tree: >> >> * easy accessibility for PG developers >> >> * at any given time we need to be using a specific "blessed" version, >> so that all developers can get equivalent results. There's pretty much >> no chance of that happening if we depend on distro-provided packages, >> even if those share a common upstream. > Yeah, but those advantages could also be gained by putting the > pgindent tree on git.postgresql.org in a separate repository. Having > it in the same repository as the actual PostgreSQL code is not > required nor, in my opinion, particularly desirable. It adds an extra step to what a developer has to do to get pgindent up and running, so it doesn't seem to me like it's helping the goal of reducing the setup overhead. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 2017-05-19 12:21:52 -0400, Robert Haas wrote: > On Fri, May 19, 2017 at 11:22 AM, Tom Lanewrote: > > I certainly would rather that our version matched something that's under > > active maintenance someplace. But it seems like there are two good > > arguments for having a copy in our tree: > > > > * easy accessibility for PG developers > > > > * at any given time we need to be using a specific "blessed" version, > > so that all developers can get equivalent results. There's pretty much > > no chance of that happening if we depend on distro-provided packages, > > even if those share a common upstream. > > Yeah, but those advantages could also be gained by putting the > pgindent tree on git.postgresql.org in a separate repository. Having > it in the same repository as the actual PostgreSQL code is not > required nor, in my opinion, particularly desirable. I'm of the contrary opinion. A lot of the regular churn due to pgindent right now is because it's inconvenient to run. Having to clone a separate repository, compile that project, put it into PATH (fun if there's multiple versions), run pgindent, discover typedefs.list is out of date, update, run, ... is pretty much a guarantee that'll continue. If we had a make indent that computed local typedefs list, *added* new but not removed old ones, we could get much closer to just always being properly indented. The cost of putting it somewhere blow src/tools/pgindent seems fairly minor. - 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, May 19, 2017 at 11:22 AM, Tom Lanewrote: > I certainly would rather that our version matched something that's under > active maintenance someplace. But it seems like there are two good > arguments for having a copy in our tree: > > * easy accessibility for PG developers > > * at any given time we need to be using a specific "blessed" version, > so that all developers can get equivalent results. There's pretty much > no chance of that happening if we depend on distro-provided packages, > even if those share a common upstream. Yeah, but those advantages could also be gained by putting the pgindent tree on git.postgresql.org in a separate repository. Having it in the same repository as the actual PostgreSQL code is not required nor, in my opinion, particularly desirable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On 05/19/2017 06:48 PM, Tom Lane wrote: Heikki Linnakangaswrites: You can get a pretty good typedefs list just by looking for the pattern "} ;". That's going to catch a lot of things that are just variables, though. It might be all right as long as there was manual filtering after it. At a quick glance, there are only a couple of them. This two cases caught my eye. In twophase.c: static struct xllist { StateFileChunk *head; /* first data block in the chain */ StateFileChunk *tail; /* last block in chain */ uint32 num_chunks; uint32 bytes_free; /* free bytes left in tail block */ uint32 total_len; /* total data bytes in chain */ } records; And this in informix.c: static struct { longval; int maxdigits; int digits; int remaining; charsign; char *val_string; } value; IMHO it would actually be an improvement if there was a space rather than a tab there. But I'm not sure what else it would mess up to consider those typedef names. And those are awfully generic names; wouldn't hurt to rename them, anyway. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
On Fri, May 19, 2017 at 11:22:54AM -0400, Tom Lane wrote: > We've had reasonably decent luck with tracking the tzcode/tzdata packages > as local copies, so I feel like we're not taking on anything unreasonable > if our model is that we'll occasionally (not oftener than once per year) > update our copy to recent upstream and then re-indent using that. I guess by having a copy in our tree we would overtly update our version, rather than downloading whatever happens to be the most recent version and finding changes by accident. If we could download a specific version that had everything we need, that would work too. > > What about perltidy itself..? We don't include that in our tree either. > > Not being much of a Perl guy, I don't care one way or the other about > perltidy. Somebody else can work on that if it needs work. We have agreed on a fixed version of perltidy and I added a download link to pgindent/README. -- Bruce Momjianhttp://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: [HACKERS] Preliminary results for proposed new pgindent implementation
Heikki Linnakangaswrites: > You can get a pretty good typedefs list just by looking for the pattern > "} ;". That's going to catch a lot of things that are just variables, though. It might be all right as long as there was manual filtering after 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On 05/19/2017 06:05 PM, Stephen Frost wrote: * Tom Lane (t...@sss.pgh.pa.us) wrote: Robert Haaswrites: On Thu, May 18, 2017 at 11:00 PM, Tom Lane wrote: The reason PGSSTrackLevel is "unrecognized" is that it's not in typedefs.list, which is a deficiency in our typedef-collection technology not in indent. (I believe the problem is that there are no variables declared with that typename, causing there to not be any of the kind of symbol table entries we are looking for.) This, however, doesn't sound so good. Isn't there some way this can be fixed? I'm intending to look into it, but I think it's mostly independent of whether we replace pgindent itself. The existing code has the same problem, really. One brute-force way we could deal with the problem is to have a "manual" list of names to be treated as typedefs, in addition to whatever the buildfarm produces. I see no other way than that to get, for instance, simplehash.h's SH_TYPE to be formatted as a typedef. There are also some typedefs that don't get formatted correctly because they are only used for wonky options that no existing typedef-reporting buildfarm member builds. Manual addition might be the path of least resistance there too. Now the other side of this coin is that, by definition, such typedefs are not getting used in a huge number of places. If we just had to live with it, it might not be awful. Dealing with the typedef lists in general is a bit of a pain to get right, to make sure that new just-written code gets correctly indented. Perhaps we could find a way to incorporate the buildfarm typedef lists and a manual list and a locally generated/provided set in a simpler fashion in general. You can get a pretty good typedefs list just by looking for the pattern "} ;". Something like this: grep -o -h -I --perl-regexp -r "}\W(\w+);" src/ contrib/ | perl -pe 's/}\W(.*);/\1/' | sort | uniq > typedefs.list It won't cover system headers and non-struct typedefs, but it catches those simplehash typedefs and PGSSTrackLevel. Maybe we should run that and merge the result with the typedef lists we collect in the buildfarm. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Stephen Frostwrites: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Yes, moving the goalposts on ease-of-use is an important consideration >> here. What that says to me is that we ought to pull FreeBSD indent >> into our tree, and provide Makefile support that makes it easy for >> any developer to build it and put it into their PATH. (I suppose >> that means support in the MSVC scripts too, but somebody else will >> have to do that part.) > I'm not a huge fan of this, however. Do we really need to carry around > the FreeBSD indent in our tree? I had been expecting that these changes > would eventually result in a package that's available in the common > distributions (possibly from apt/yum.postgresql.org, at least until it's > in the main Debian-based and RHEL-based package systems). Are you > thinking that we'll always have to have our own modified version? I certainly would rather that our version matched something that's under active maintenance someplace. But it seems like there are two good arguments for having a copy in our tree: * easy accessibility for PG developers * at any given time we need to be using a specific "blessed" version, so that all developers can get equivalent results. There's pretty much no chance of that happening if we depend on distro-provided packages, even if those share a common upstream. We've had reasonably decent luck with tracking the tzcode/tzdata packages as local copies, so I feel like we're not taking on anything unreasonable if our model is that we'll occasionally (not oftener than once per year) update our copy to recent upstream and then re-indent using that. > What about perltidy itself..? We don't include that in our tree either. Not being much of a Perl guy, I don't care one way or the other about perltidy. Somebody else can work on that if it needs work. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
Tom, all, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Robert Haaswrites: > > On Thu, May 18, 2017 at 11:00 PM, Tom Lane wrote: > >> The reason PGSSTrackLevel is "unrecognized" is that it's not in > >> typedefs.list, which is a deficiency in our typedef-collection > >> technology not in indent. (I believe the problem is that there > >> are no variables declared with that typename, causing there to > >> not be any of the kind of symbol table entries we are looking for.) > > > This, however, doesn't sound so good. Isn't there some way this can be > > fixed? > > I'm intending to look into it, but I think it's mostly independent of > whether we replace pgindent itself. The existing code has the same > problem, really. > > One brute-force way we could deal with the problem is to have a "manual" > list of names to be treated as typedefs, in addition to whatever the > buildfarm produces. I see no other way than that to get, for instance, > simplehash.h's SH_TYPE to be formatted as a typedef. There are also > some typedefs that don't get formatted correctly because they are only > used for wonky options that no existing typedef-reporting buildfarm member > builds. Manual addition might be the path of least resistance there too. > > Now the other side of this coin is that, by definition, such typedefs > are not getting used in a huge number of places. If we just had to > live with it, it might not be awful. Dealing with the typedef lists in general is a bit of a pain to get right, to make sure that new just-written code gets correctly indented. Perhaps we could find a way to incorporate the buildfarm typedef lists and a manual list and a locally generated/provided set in a simpler fashion in general. > >> All in all, this looks pretty darn good from here, and I'm thinking > >> we should push forward on it. > > > What does that exactly mean concretely? > > That means I plan to continue putting effort into it with the goal of > making a switchover sometime pretty darn soon. We do not have a very > wide window for fooling with pgindent rules, IMO --- once v11 development > starts I think we can't touch it again (until this time next year). Agreed. > > We've talked about pulling pgindent into our main repo, or posting a > > link to a tarball someplace. An intermediate plan might be to give it > > its own repo, but on git.postgresql.org, which seems like it might > > give us the best of both worlds. But I really want something that's > > going to be easy to set up and configure. It took me years to be > > brave enough to get the current pgindent set up. > > Yes, moving the goalposts on ease-of-use is an important consideration > here. What that says to me is that we ought to pull FreeBSD indent > into our tree, and provide Makefile support that makes it easy for > any developer to build it and put it into their PATH. (I suppose > that means support in the MSVC scripts too, but somebody else will > have to do that part.) I'm not a huge fan of this, however. Do we really need to carry around the FreeBSD indent in our tree? I had been expecting that these changes would eventually result in a package that's available in the common distributions (possibly from apt/yum.postgresql.org, at least until it's in the main Debian-based and RHEL-based package systems). Are you thinking that we'll always have to have our own modified version? > We should also think hard about getting rid of the entab dependency, > to eliminate the other nonstandard prerequisite program. We could > either accept that that will result in some tab-vs-space changes in > our code, or try to convert those steps in pgindent into pure Perl, > or try to convince Piotr to add an option to indent that will make > it do tabs the way we want (ie use a space not a tab if the tab > would only move one space anyway). What about perltidy itself..? We don't include that in our tree either. I do think it'd be good to if Piotr would add such an option, hopefully that's agreeable. > Lastly (and I've said this before, but you pushed back on it at > the time), if we're doing this then we're going all in. That > means reformatting the back branches to match too. That diff > is already big enough to be a disaster for back-patching, and > we haven't even considered whether we want to let pgindent adopt > less-inconsistent rules for comment indentation. So I think that > as soon as the dust has settled in HEAD, we back-patch the addition > of FreeBSD indent, and the changes in pgindent proper, and then > run pgindent in each supported back branch. Ugh. This would be pretty painful, but I agree that back-patching without doing re-indenting the back-branches would also suck, so I'm on the fence about this. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Robert Haaswrites: > On Thu, May 18, 2017 at 11:00 PM, Tom Lane wrote: >> The reason PGSSTrackLevel is "unrecognized" is that it's not in >> typedefs.list, which is a deficiency in our typedef-collection >> technology not in indent. (I believe the problem is that there >> are no variables declared with that typename, causing there to >> not be any of the kind of symbol table entries we are looking for.) > This, however, doesn't sound so good. Isn't there some way this can be fixed? I'm intending to look into it, but I think it's mostly independent of whether we replace pgindent itself. The existing code has the same problem, really. One brute-force way we could deal with the problem is to have a "manual" list of names to be treated as typedefs, in addition to whatever the buildfarm produces. I see no other way than that to get, for instance, simplehash.h's SH_TYPE to be formatted as a typedef. There are also some typedefs that don't get formatted correctly because they are only used for wonky options that no existing typedef-reporting buildfarm member builds. Manual addition might be the path of least resistance there too. Now the other side of this coin is that, by definition, such typedefs are not getting used in a huge number of places. If we just had to live with it, it might not be awful. >> All in all, this looks pretty darn good from here, and I'm thinking >> we should push forward on it. > What does that exactly mean concretely? That means I plan to continue putting effort into it with the goal of making a switchover sometime pretty darn soon. We do not have a very wide window for fooling with pgindent rules, IMO --- once v11 development starts I think we can't touch it again (until this time next year). > We've talked about pulling pgindent into our main repo, or posting a > link to a tarball someplace. An intermediate plan might be to give it > its own repo, but on git.postgresql.org, which seems like it might > give us the best of both worlds. But I really want something that's > going to be easy to set up and configure. It took me years to be > brave enough to get the current pgindent set up. Yes, moving the goalposts on ease-of-use is an important consideration here. What that says to me is that we ought to pull FreeBSD indent into our tree, and provide Makefile support that makes it easy for any developer to build it and put it into their PATH. (I suppose that means support in the MSVC scripts too, but somebody else will have to do that part.) We should also think hard about getting rid of the entab dependency, to eliminate the other nonstandard prerequisite program. We could either accept that that will result in some tab-vs-space changes in our code, or try to convert those steps in pgindent into pure Perl, or try to convince Piotr to add an option to indent that will make it do tabs the way we want (ie use a space not a tab if the tab would only move one space anyway). Lastly (and I've said this before, but you pushed back on it at the time), if we're doing this then we're going all in. That means reformatting the back branches to match too. That diff is already big enough to be a disaster for back-patching, and we haven't even considered whether we want to let pgindent adopt less-inconsistent rules for comment indentation. So I think that as soon as the dust has settled in HEAD, we back-patch the addition of FreeBSD indent, and the changes in pgindent proper, and then run pgindent in each supported back branch. 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: [HACKERS] Preliminary results for proposed new pgindent implementation
On Thu, May 18, 2017 at 11:00 PM, Tom Lanewrote: > * Improvements in formatting around sizeof and related constructs, > for example: > > * Likewise, operators after casts work better than before: > > * Sane formatting of function typedefs, for example: > > * Non-typedef struct pointers are now formatted consistently, for example: > > * Better handling of pointers with const/volatile qualifiers, for example: > > * Better handling of PG_USED_FOR_ASSERTS_ONLY declarations, for example > > * Corner cases where no space was left before a comment are fixed: Those all sound like good things. > Another set of changes is slightly different handling of unrecognized > typedef names: > > @@ -250,7 +250,7 @@ typedef enum > PGSS_TRACK_NONE,/* track no statements */ > PGSS_TRACK_TOP, /* only top level statements */ > PGSS_TRACK_ALL /* all statements, including nested ones */ > -} PGSSTrackLevel; > +} PGSSTrackLevel; > > The reason PGSSTrackLevel is "unrecognized" is that it's not in > typedefs.list, which is a deficiency in our typedef-collection > technology not in indent. (I believe the problem is that there > are no variables declared with that typename, causing there to > not be any of the kind of symbol table entries we are looking for.) > The handling of such names was already slightly wonky, though; > note that the previous version was already differently indented > from what would happen if PGSSTrackLevel were known. This, however, doesn't sound so good. Isn't there some way this can be fixed? > All in all, this looks pretty darn good from here, and I'm thinking > we should push forward on it. What does that exactly mean concretely? We've talked about pulling pgindent into our main repo, or posting a link to a tarball someplace. An intermediate plan might be to give it its own repo, but on git.postgresql.org, which seems like it might give us the best of both worlds. But I really want something that's going to be easy to set up and configure. It took me years to be brave enough to get the current pgindent set up. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Preliminary results for proposed new pgindent implementation
Tom, all, * Tom Lane (t...@sss.pgh.pa.us) wrote: > All in all, this looks pretty darn good from here, and I'm thinking > we should push forward on it. +1. Thanks! Stephen signature.asc Description: Digital signature