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
Attachments:
pgindent-cleanup.patchapplication/x-download; name=pgindent-cleanup.patchDownload
diff --git a/contrib/pageinspect/heapfuncs.c b/contrib/pageinspect/heapfuncs.c
index b7d75b0..d0c2886 100644
--- a/contrib/pageinspect/heapfuncs.c
+++ b/contrib/pageinspect/heapfuncs.c
@@ -229,11 +229,12 @@ heap_page_items(PG_FUNCTION_ARGS)
{
if (tuphdr->t_infomask & HEAP_HASNULL)
{
- int bits_len =
- ((tuphdr->t_infomask2 & HEAP_NATTS_MASK) / 8 + 1) * 8;
+ int bits_len;
+ bits_len =
+ ((tuphdr->t_infomask2 & HEAP_NATTS_MASK) / 8 + 1) * 8;
values[11] = CStringGetTextDatum(
- bits_to_text(tuphdr->t_bits, bits_len));
+ bits_to_text(tuphdr->t_bits, bits_len));
}
else
nulls[11] = true;
diff --git a/src/backend/executor/execParallel.c b/src/backend/executor/execParallel.c
index bdc6b43..6df62a7 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -400,8 +400,8 @@ ExecInitParallelPlan(PlanState *planstate, EState *estate, int nworkers)
if (estate->es_instrument)
{
instrumentation_len =
- offsetof(SharedExecutorInstrumentation, plan_node_id)
- + sizeof(int) * e.nnodes;
+ offsetof(SharedExecutorInstrumentation, plan_node_id) +
+ sizeof(int) * e.nnodes;
instrumentation_len = MAXALIGN(instrumentation_len);
instrument_offset = instrumentation_len;
instrumentation_len += sizeof(Instrumentation) * e.nnodes * nworkers;
@@ -513,7 +513,7 @@ ExecParallelRetrieveInstrumentation(PlanState *planstate,
/* Also store the per-worker detail. */
ibytes = instrumentation->num_workers * sizeof(Instrumentation);
planstate->worker_instrument =
- palloc(offsetof(WorkerInstrumentation, instrument) + ibytes);
+ palloc(ibytes + offsetof(WorkerInstrumentation, instrument));
planstate->worker_instrument->num_workers = instrumentation->num_workers;
memcpy(&planstate->worker_instrument->instrument, instrument, ibytes);
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 614b26b..0c1e4a3 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -3016,7 +3016,8 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
Expr *transfnexpr;
/*
- * Set up infrastructure for calling the transfn
+ * Set up infrastructure for calling the transfn. Note that invtrans
+ * is not needed here.
*/
build_aggregate_transfn_expr(inputTypes,
numArguments,
@@ -3025,7 +3026,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
aggtranstype,
aggref->inputcollid,
aggtransfn,
- InvalidOid, /* invtrans is not needed here */
+ InvalidOid,
&transfnexpr,
NULL);
fmgr_info(aggtransfn, &pertrans->transfn);
diff --git a/src/backend/tsearch/spell.c b/src/backend/tsearch/spell.c
index 5f1a97e..f48664b 100644
--- a/src/backend/tsearch/spell.c
+++ b/src/backend/tsearch/spell.c
@@ -1338,9 +1338,10 @@ NIImportOOAffixes(IspellDict *Conf, const char *filename)
|| (sflaglen > 2 && Conf->flagMode == FM_LONG))
goto nextline;
- /*
+ /*--------
* Affix header. For example:
* SFX \ N 1
+ *--------
*/
if (fields_read == 4)
{
@@ -1350,9 +1351,10 @@ NIImportOOAffixes(IspellDict *Conf, const char *filename)
else
flagflags = 0;
}
- /*
+ /*--------
* Affix fields. For example:
- * SFX \ 0 Y/L [^Y]
+ * SFX \ 0 Y/L [^Y]
+ *--------
*/
else
{
diff --git a/src/backend/tsearch/to_tsany.c b/src/backend/tsearch/to_tsany.c
index 3f69d74..d41f82c 100644
--- a/src/backend/tsearch/to_tsany.c
+++ b/src/backend/tsearch/to_tsany.c
@@ -311,7 +311,8 @@ pushval_morph(Datum opaque, TSQueryParserState state, char *strval, int lenval,
}
}
- pos = prs.words[count].pos.pos; /* save current word's position */
+ /* save current word's position */
+ pos = prs.words[count].pos.pos;
/* Go through all variants obtained from this token */
cntvar = 0;
@@ -343,7 +344,11 @@ pushval_morph(Datum opaque, TSQueryParserState state, char *strval, int lenval,
}
if (cntpos)
- pushOperator(state, data->qoperator, 1); /* distance may be useful */
+ {
+ /* distance may be useful */
+ pushOperator(state, data->qoperator, 1);
+ }
+
cntpos++;
}
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 1db4acf..4ec2fed 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -817,11 +817,14 @@ pg_size_bytes(PG_FUNCTION_ARGS)
else if (pg_strcasecmp(strptr, "kb") == 0)
multiplier = (int64) 1024;
else if (pg_strcasecmp(strptr, "mb") == 0)
- multiplier = (int64) 1024 * 1024;
+ multiplier = ((int64) 1024) * 1024;
+
else if (pg_strcasecmp(strptr, "gb") == 0)
- multiplier = (int64) 1024 * 1024 * 1024;
+ multiplier = ((int64) 1024) * 1024 * 1024;
+
else if (pg_strcasecmp(strptr, "tb") == 0)
- multiplier = (int64) 1024 * 1024 * 1024 * 1024;
+ multiplier = ((int64) 1024) * 1024 * 1024 * 1024;
+
else
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
diff --git a/src/backend/utils/adt/tsginidx.c b/src/backend/utils/adt/tsginidx.c
index 3f1e7f9..ebc11c9 100644
--- a/src/backend/utils/adt/tsginidx.c
+++ b/src/backend/utils/adt/tsginidx.c
@@ -222,9 +222,10 @@ TS_execute_ternary(GinChkVal *gcv, QueryItem *curitem)
check_stack_depth();
if (curitem->type == QI_VAL)
- return checkcondition_gin_internal(gcv,
- (QueryOperand *) curitem,
- NULL /* don't have any position info */);
+ return
+ checkcondition_gin_internal(gcv,
+ (QueryOperand *) curitem,
+ NULL /* don't have position info */ );
switch (curitem->qoperator.oper)
{
diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index d4fb0ff..8298e38 100644
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -834,7 +834,8 @@ tsvector_filter(PG_FUNCTION_ARGS)
posvout->pos[npos++] = posvin->pos[k];
}
- if (!npos) /* no satisfactory positions found, so skip that lexeme */
+ /* if no satisfactory positions found, skip lexeme */
+ if (!npos)
continue;
arrout[j].haspos = true;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
- 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers