Обсуждение: pgindent fixups
I spent some time going through the output of a trial pgindent run today. Some questions/problems: 1. Is pgindent supposed to touch DATA() lines? Because it does. 2. CustomPathMethods is not in the buildfarm's typedefs.list. Why not? I'm attaching a patch that fixes up a few other problems that I found, which I'll commit if there are not objections. In detail: - In contrib/pageinspect/heapfuncs.c, it separates the declaration of bits_len from the initialization to avoid an awkward line-wrap. - In src/backend/executor/execParallel.c, it dodges two cases where pgindent does stupid things with offsetof. Apparently, pgindent thinks that you should write "offsetof(a, b) +c" rather than "offsetof(a, b) + c". In one case, I talked it out of it by putting the + at the end of the first line rather than the start of the continuation line. The other statement was all on one line so I changed it to say "c + offsetof(a, b)" instead. - In nodeAgg.c, to_ts_any.c, and tsvector_op.c, I moved end-of-line comments to their own separate lines, because they were getting broken up into multiple lines in ways that seemed awkward. In tsginidx.c, I left a similar comment inline but fiddled the whitespace and comment text to avoid getting a line break in mid-comment. - In spell.c, I added -------- markers around a comment to prevent pgindent from messing with the whitespace (entab still adjusts it, but that should look the same if you have your tab stops set right). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
On 05/02/2016 10:56 PM, Robert Haas wrote: > I spent some time going through the output of a trial pgindent run > today. Some questions/problems: > > 1. Is pgindent supposed to touch DATA() lines? Because it does. Apart from being detabbed/entabbed, no. pgindent protects (or tries to protect) DATA and CATALOG lines by enclosng them in comments which it later removes. > > 2. CustomPathMethods is not in the buildfarm's typedefs.list. Why not? Because it's not used anywhere. typdefs get into the list by being used and thus having a typedef symbol in the compiled binary. Just creating a typedef isn't enough. This has happened before. You can get some insight into how the typedefs list is generated by looking here: <http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html> cheers andrew
Robert Haas <robertmhaas@gmail.com> writes:
> 1. Is pgindent supposed to touch DATA() lines? Because it does.
It always has.
> 2. CustomPathMethods is not in the buildfarm's typedefs.list. Why not?
Probably because there are no variables, parameters, or fields declared
with that typedef, so it doesn't get into the debugger symbol table of
any .o file. Grep says that the single use of the struct doesn't use
the typedef; it'sconst struct CustomPathMethods *methods;
So you'd need to change that, or else tweak some code somewhere to have
a variable explicitly declared using the typedef.
> - In src/backend/executor/execParallel.c, it dodges two cases where
> pgindent does stupid things with offsetof.
Well, it's also pretty stupid about sizeof, or casts generally, so
I'm not really convinced you need to get exercised about these two
places in particular. But no objection to tweaking them if you
want to.
regards, tom lane
On Tue, May 3, 2016 at 9:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> 1. Is pgindent supposed to touch DATA() lines? Because it does. > > It always has. > >> 2. CustomPathMethods is not in the buildfarm's typedefs.list. Why not? > > Probably because there are no variables, parameters, or fields declared > with that typedef, so it doesn't get into the debugger symbol table of > any .o file. Grep says that the single use of the struct doesn't use > the typedef; it's > const struct CustomPathMethods *methods; > So you'd need to change that, or else tweak some code somewhere to have > a variable explicitly declared using the typedef. Ah. It's a minor issue, so probably not worth worrying about. >> - In src/backend/executor/execParallel.c, it dodges two cases where >> pgindent does stupid things with offsetof. > > Well, it's also pretty stupid about sizeof, or casts generally, so > I'm not really convinced you need to get exercised about these two > places in particular. But no objection to tweaking them if you > want to. OK, I committed this. Barring objections, I'll go ahead and pgindent the whole tree tomorrow. If we're going to revert anything big then we might want to hold off, but otherwise I think its better to get this done sooner rather than later. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> OK, I committed this. Barring objections, I'll go ahead and pgindent
> the whole tree tomorrow. If we're going to revert anything big then
> we might want to hold off, but otherwise I think its better to get
> this done sooner rather than later.
Well, there are at least two patchsets we're actively discussing
reverting, so I think this should wait till those decisions are resolved.
regards, tom lane
On Tue, May 3, 2016 at 11:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> OK, I committed this. Barring objections, I'll go ahead and pgindent >> the whole tree tomorrow. If we're going to revert anything big then >> we might want to hold off, but otherwise I think its better to get >> this done sooner rather than later. > > Well, there are at least two patchsets we're actively discussing > reverting, so I think this should wait till those decisions are resolved. OK, but that may well mean we don't get this done before beta1, which I think is a bummer, but oh well. There are a lot more than 2 patchsets that are busted at the moment, unfortunately, but I assume you are referring to "snapshot too old" and "Use Foreign Key relationships to infer multi-column join selectivity". -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 3, 2016 at 11:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, there are at least two patchsets we're actively discussing
>> reverting, so I think this should wait till those decisions are resolved.
> OK, but that may well mean we don't get this done before beta1, which
> I think is a bummer, but oh well.
pgindent is reliable enough that I'm not really worried about running
it post-beta. Obviously sooner is better than later, to give authors
of 9.7 patches more time to rebase; but I do not think we should give
ourselves extra work just to do it a little sooner.
> There are a lot more than 2 patchsets that are busted at the moment,
> unfortunately, but I assume you are referring to "snapshot too old"
> and "Use Foreign Key relationships to infer multi-column join
> selectivity".
Yeah, those are the ones I'm thinking of. I've not heard serious
proposals to revert any others, have you?
regards, tom lane
On Tue, May 3, 2016 at 11:31 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, May 3, 2016 at 11:23 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> OK, I committed this. Barring objections, I'll go ahead and pgindent >>> the whole tree tomorrow. If we're going to revert anything big then >>> we might want to hold off, but otherwise I think its better to get >>> this done sooner rather than later. >> >> Well, there are at least two patchsets we're actively discussing >> reverting, so I think this should wait till those decisions are resolved. > > OK, but that may well mean we don't get this done before beta1, which > I think is a bummer, but oh well. So I really would like to get a pgindent run done. Any objections to doing it sometime RSN? It is of course possible that it might make something that we want to revert later harder to revert, but I think we should just accept that risk and move forward. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> So I really would like to get a pgindent run done. Any objections to
> doing it sometime RSN? It is of course possible that it might make
> something that we want to revert later harder to revert, but I think
> we should just accept that risk and move forward.
Now that we bit the bullet on 137805f89, I do not think there's anything
else with a really high probability of being reverted. Might as well do
the run. Please note that typedefs.list is already out of date.
regards, tom lane