pgindent && weirdness
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 861a9148ed..fff54062b0 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1405,13 +1405,13 @@ examine_opclause_expression(OpExpr *expr, Var **varp, Const **cstp, bool *varonl
if (IsA(rightop, RelabelType))
rightop = (Node *) ((RelabelType *) rightop)->arg;- if (IsA(leftop, Var) && IsA(rightop, Const))
+ if (IsA(leftop, Var) &&IsA(rightop, Const))
{
var = (Var *) leftop;
cst = (Const *) rightop;
varonleft = true;
}
- else if (IsA(leftop, Const) && IsA(rightop, Var))
+ else if (IsA(leftop, Const) &&IsA(rightop, Var))
{
var = (Var *) rightop;
cst = (Const *) leftop;This seems a really strange change; this
git grep '&&[^([:space:]]' -- *.c
shows that we already have a dozen or so occurrences already. (That's
ignoring execExprInterp.c's use of computed gotos.)
I don't care all that much, but wanted to throw it out in case somebody
is specifically interested in studying pgindent's logic, since the last
round of changes has yielded excellent results.
Thanks,
--
�lvaro Herrera PostgreSQL Expert, https://www.2ndQuadrant.com/
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:
- if (IsA(leftop, Var) && IsA(rightop, Const)) + if (IsA(leftop, Var) &&IsA(rightop, Const))
Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.
It'd be nice to fix properly, but I've tended to take the path
of least resistance by breaking such lines to avoid the ugliness:
if (IsA(leftop, Var) &&
IsA(rightop, Const))
regards, tom lane
On Tue, Jan 14, 2020 at 05:30:21PM -0500, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:- if (IsA(leftop, Var) && IsA(rightop, Const)) + if (IsA(leftop, Var) &&IsA(rightop, Const))Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.It'd be nice to fix properly, but I've tended to take the path
of least resistance by breaking such lines to avoid the ugliness:if (IsA(leftop, Var) &&
IsA(rightop, Const))
In the past I would use a post-processing step after BSD indent to fix
up these problems.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:- if (IsA(leftop, Var) && IsA(rightop, Const)) + if (IsA(leftop, Var) &&IsA(rightop, Const))Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.It'd be nice to fix properly, but I've tended to take the path
of least resistance by breaking such lines to avoid the ugliness:if (IsA(leftop, Var) &&
IsA(rightop, Const))
I am on vacation away from the Internet this week but somehow saw this
on my phone and couldn't stop myself from peeking at pg_bsd_ident
again. Yeah, "(Var)" (where Var is a known typename) causes it to
think that any following operator must be unary.
One way to fix that in the cases Alvaro is referring to is to tell
override the setting so that && (and likewise ||) are never considered
to be unary, though I haven't tested this much and there are surely
other ways to achieve this:
diff --git a/lexi.c b/lexi.c
index d43723c..6de3227 100644
--- a/lexi.c
+++ b/lexi.c
@@ -655,6 +655,12 @@ stop_lit:
unary_delim = state->last_u_d;
break;
}
+
+ /* && and || are never unary */
+ if ((token[0] == '&' && *buf_ptr == '&') ||
+ (token[0] == '|' && *buf_ptr == '|'))
+ state->last_u_d = false;
+
while (*(e_token - 1) == *buf_ptr || *buf_ptr == '=') {
/*
* handle ||, &&, etc, and also things as in int *****iThe problem with that is that && sometimes *should* be formatted like
a unary operator: when it's part of the nonstandard GCC computed goto
syntax.
On Thu, Jan 16, 2020 at 3:59 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.
Here's a better fix:
diff --git a/indent.c b/indent.c
index 9faf57a..51a60a6 100644
--- a/indent.c
+++ b/indent.c
@@ -570,8 +570,11 @@ check_type:
ps.in_or_st = false; /* turn off flag for structure decl or
* initialization */
}
- /* parenthesized type following sizeof or offsetof is not a cast */
- if (ps.keyword == 1 || ps.keyword == 2)
+ /*
+ * parenthesized type following sizeof or offsetof is
not a cast;
+ * likewise for function-like macros that take a type
+ */
+ if (ps.keyword == 1 || ps.keyword == 2 || ps.last_token == ident)
ps.not_cast_mask |= 1 << ps.p_l_follow;
break;On 2020-Jan-17, Thomas Munro wrote:
On Thu, Jan 16, 2020 at 3:59 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.Here's a better fix:
This is indeed a very good fix! Several badly formatted sites in our
code are improved with this change.
Thanks,
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jan 16, 2020 at 06:13:36PM -0300, Alvaro Herrera wrote:
This is indeed a very good fix! Several badly formatted sites in our
code are improved with this change.
Nice find! Could you commit that? I can see many places improved as
well, among explain.c, tablecmds.c, typecmds.c, and much more.
--
Michael
On 2020-Jan-17, Michael Paquier wrote:
On Thu, Jan 16, 2020 at 06:13:36PM -0300, Alvaro Herrera wrote:
This is indeed a very good fix! Several badly formatted sites in our
code are improved with this change.Nice find! Could you commit that? I can see many places improved as
well, among explain.c, tablecmds.c, typecmds.c, and much more.
I think Tom is the only one who can commit that,
https://git.postgresql.org/gitweb/?p=pg_bsd_indent.git
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Jan-17, Michael Paquier wrote:
Nice find! Could you commit that? I can see many places improved as
well, among explain.c, tablecmds.c, typecmds.c, and much more.
I think Tom is the only one who can commit that,
https://git.postgresql.org/gitweb/?p=pg_bsd_indent.git
I don't *think* that repo is locked down that hard --- IIRC,
PG committers should have access to it. But I was hoping to
hear Piotr's opinion before moving forward.
regards, tom lane
On 2020-Jan-16, Tom Lane wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Jan-17, Michael Paquier wrote:
Nice find! Could you commit that? I can see many places improved as
well, among explain.c, tablecmds.c, typecmds.c, and much more.I think Tom is the only one who can commit that,
https://git.postgresql.org/gitweb/?p=pg_bsd_indent.gitI don't *think* that repo is locked down that hard --- IIRC,
PG committers should have access to it. But I was hoping to
hear Piotr's opinion before moving forward.
FWIW I think this code predates Piotr's involvement, I think; at least,
it was already there in the FreeBSD code he imported:
https://github.com/pstef/freebsd_indent/commit/55c29a8774923f2d40fef7919b9490f61e57e7bb#diff-85c94ae15198235e2363f96216b9a1b2R565
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Jan-16, Tom Lane wrote:
... But I was hoping to
hear Piotr's opinion before moving forward.
FWIW I think this code predates Piotr's involvement, I think; at least,
it was already there in the FreeBSD code he imported:
https://github.com/pstef/freebsd_indent/commit/55c29a8774923f2d40fef7919b9490f61e57e7bb#diff-85c94ae15198235e2363f96216b9a1b2R565
The roots of that code are even older than Postgres, I believe,
and there may not be anybody left who understands it completely.
But Piotr has certainly spent more time looking at it than I have,
so I'd still like to hear what he thinks.
regards, tom lane
On Fri, Jan 17, 2020 at 3:58 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-Jan-16, Tom Lane wrote:
... But I was hoping to
hear Piotr's opinion before moving forward.
Me too.
Thinking about this again: It's obviously not true that everything
that looks like a function call is not a cast. You could have
"my_cast(Type)" that expands to "(Type)" or some slightly more useful
variant of that, and then "my_cast(Type) -1" would, with this patch
applied, be reformatted as "my_cast(Type) - 1" because it'd err on the
side of thinking that the expression produces a value and therefore
the minus sign must be a binary operator that needs whitespace on both
sides, and that'd be wrong. However, it seems to me that macros that
expand to raw cast syntax (and I mean just "(Type)", not a complete
cast including the value to be cast, like "((Type) (x))") must be rare
and unusual things, and I think it's better to err on the side of
thinking that function-like macros are values, not casts. That's all
the change does, and fortunately the authors of indent showed how to
do that with their existing special cases for offsetof and sizeof; I'm
just extending that treatment to any identifier.
Is there some other case I'm not thinking of that is confused by the
change? I'm sure you could contrive something it screws up, but my
question is about real code that people would actually write. Piotr,
is there an easy way to reindent some large non-PostgreSQL body of
code that uses a cousin of this code to see if it gets better or worse
with the change?
On 2020-Feb-17, Thomas Munro wrote:
Thinking about this again: It's obviously not true that everything
that looks like a function call is not a cast. You could have
"my_cast(Type)" that expands to "(Type)" or some slightly more useful
variant of that, and then "my_cast(Type) -1" would, with this patch
applied, be reformatted as "my_cast(Type) - 1" because it'd err on the
side of thinking that the expression produces a value and therefore
the minus sign must be a binary operator that needs whitespace on both
sides, and that'd be wrong. However, it seems to me that macros that
expand to raw cast syntax (and I mean just "(Type)", not a complete
cast including the value to be cast, like "((Type) (x))") must be rare
and unusual things, and I think it's better to err on the side of
thinking that function-like macros are values, not casts. That's all
the change does, and fortunately the authors of indent showed how to
do that with their existing special cases for offsetof and sizeof; I'm
just extending that treatment to any identifier.
Hmm ... this suggests to me that if you remove these alleged special
cases for offsetof and sizeof, the new code handles them correctly
anyway. Do you think it's worth giving that a try? Not because
removing the special cases would have any value, but rather to see if
anything interesting pops up.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Feb 18, 2020 at 4:35 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
Hmm ... this suggests to me that if you remove these alleged special
cases for offsetof and sizeof, the new code handles them correctly
anyway. Do you think it's worth giving that a try? Not because
removing the special cases would have any value, but rather to see if
anything interesting pops up.
Good thought, since keywords also have last_token == ident so it's
redundant to check the keyword. But while testing that I realised
that either way we get this wrong:
- return (int) *s1 - (int) *s2;
+ return (int) * s1 - (int) *s2;So I think the right formulation is one that allows offsetof and
sizeof to receive not-a-cast treatment, but not any other known
keyword:
diff --git a/indent.c b/indent.c
index 9faf57a..ed6dce2 100644
--- a/indent.c
+++ b/indent.c
@@ -570,8 +570,15 @@ check_type:
ps.in_or_st = false; /* turn off flag for structure decl or
* initialization */
}
- /* parenthesized type following sizeof or offsetof is not a cast */
- if (ps.keyword == 1 || ps.keyword == 2)
+ /*
+ * parenthesized type following sizeof or offsetof is not a
+ * cast; we also assume the same about similar macros,
+ * so if there is any other non-keyword identifier immediately
+ * preceding a type name in parens we won't consider it to be
+ * a cast
+ */
+ if (ps.last_token == ident &&
+ (ps.keyword == 0 || ps.keyword == 1 || ps.keyword == 2))
ps.not_cast_mask |= 1 << ps.p_l_follow;
break;Another problem is that there is one thing in our tree that looks like
a non-cast under the new rule, but it actually expands to a type name,
so now we get that wrong! (I mean, unpatched indent doesn't really
understand it either, it thinks it's a cast, but at least it knows the
following * is not a binary operator):
- STACK_OF(X509_NAME) *root_cert_list = NULL;
+ STACK_OF(X509_NAME) * root_cert_list = NULL;That's a macro from an OpenSSL header. Not sure what to do about that.
Thomas Munro <thomas.munro@gmail.com> writes:
Another problem is that there is one thing in our tree that looks like
a non-cast under the new rule, but it actually expands to a type name,
so now we get that wrong! (I mean, unpatched indent doesn't really
understand it either, it thinks it's a cast, but at least it knows the
following * is not a binary operator):
- STACK_OF(X509_NAME) *root_cert_list = NULL; + STACK_OF(X509_NAME) * root_cert_list = NULL;
That's a macro from an OpenSSL header. Not sure what to do about that.
If we get that wrong, but a hundred other places look better,
I'm not too fussed about it.
regards, tom lane
On Tue, Feb 18, 2020 at 12:42 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
Another problem is that there is one thing in our tree that looks like
a non-cast under the new rule, but it actually expands to a type name,
so now we get that wrong! (I mean, unpatched indent doesn't really
understand it either, it thinks it's a cast, but at least it knows the
following * is not a binary operator):- STACK_OF(X509_NAME) *root_cert_list = NULL; + STACK_OF(X509_NAME) * root_cert_list = NULL;That's a macro from an OpenSSL header. Not sure what to do about that.
If we get that wrong, but a hundred other places look better,
I'm not too fussed about it.
Here's the patch I propose to commit to pg_bsd_indent, if the repo
lets me, and here's the result of running it on the PG tree today.
I suppose the principled way to fix that problem with STACK_OF(x)
would be to have a user-supplied list of function-like-macros that
expand to a type name, but I'm not planning to waste time on that.
Attachments:
0001-Fix-formatting-of-macros-that-take-types.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-formatting-of-macros-that-take-types.patchDownload
From d3c9c8aa8f785266dd4302f8ab980c9a6ce99b5b Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 16 May 2020 09:26:10 +1200
Subject: [PATCH] Fix formatting of macros that take types.
Previously, we would assume that a macro like IsA() in the following
example was a cast just because it mentions a known type between parens,
and that messed up the formatting of any binary operator that followed:
if (IsA(outer_path, UniquePath) ||path->skip_mark_restore)
This change errs on the side of assuming that function-like macros are
similar to sizeof() and offsetof(), so that operators are formatted
correctly:
if (IsA(outer_path, UniquePath) || path->skip_mark_restore)
Discussion: https://postgr.es/m/20200114221814.GA19630%40alvherre.pgsql
---
indent.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/indent.c b/indent.c
index 9faf57a..29b3fa4 100644
--- a/indent.c
+++ b/indent.c
@@ -570,8 +570,13 @@ check_type:
ps.in_or_st = false; /* turn off flag for structure decl or
* initialization */
}
- /* parenthesized type following sizeof or offsetof is not a cast */
- if (ps.keyword == 1 || ps.keyword == 2)
+ /*
+ * parenthesized type following sizeof or offsetof is not a cast,
+ * and we assume the same for any other non-keyword identifier,
+ * to support macros that take types
+ */
+ if (ps.last_token == ident &&
+ (ps.keyword == 0 || ps.keyword == 1 || ps.keyword == 2))
ps.not_cast_mask |= 1 << ps.p_l_follow;
break;
--
2.20.1
0001-Fix-formatting-of-IsA-and-similar-macros.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-formatting-of-IsA-and-similar-macros.patchDownload
From bd2751742a47c4796c6bb73442a6109985e03bba Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 16 May 2020 09:44:43 +1200
Subject: [PATCH] Fix formatting of IsA() and similar macros.
Fix the formatting of many places where macros and sometimes even
function calls confused pg_bsd_indent by mentioning type names in
between parentheses. We've now patched pg_bsd_indent to be less
confused about that.
This leads to a couple of places where OpenSSL's STACK_OF() macro is not
formatted correctly, but that seems like a good trade-off.
Discussion: https://postgr.es/m/20200114221814.GA19630%40alvherre.pgsql
---
src/backend/access/nbtree/nbtutils.c | 6 +++---
src/backend/commands/explain.c | 4 ++--
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/typecmds.c | 2 +-
src/backend/executor/nodeAgg.c | 2 +-
src/backend/executor/nodeProjectSet.c | 4 ++--
src/backend/libpq/be-secure-openssl.c | 2 +-
src/backend/nodes/outfuncs.c | 2 +-
src/backend/optimizer/path/costsize.c | 2 +-
src/backend/optimizer/plan/setrefs.c | 2 +-
src/backend/optimizer/util/pathnode.c | 2 +-
src/backend/parser/analyze.c | 4 ++--
src/backend/parser/parse_agg.c | 4 ++--
src/backend/parser/parse_clause.c | 2 +-
src/backend/parser/parse_expr.c | 2 +-
src/backend/statistics/extended_stats.c | 4 ++--
src/backend/utils/adt/datetime.c | 2 +-
src/backend/utils/adt/formatting.c | 2 +-
src/backend/utils/adt/numeric.c | 2 +-
src/backend/utils/adt/ruleutils.c | 6 +++---
src/backend/utils/adt/timestamp.c | 2 +-
src/backend/utils/adt/varbit.c | 2 +-
src/backend/utils/adt/varchar.c | 2 +-
src/bin/pg_dump/pg_backup_directory.c | 2 +-
src/bin/psql/tab-complete.c | 2 +-
src/common/jsonapi.c | 2 +-
src/interfaces/ecpg/ecpglib/execute.c | 2 +-
src/interfaces/ecpg/ecpglib/prepare.c | 2 +-
src/interfaces/libpq/fe-secure-openssl.c | 2 +-
src/pl/plpgsql/src/pl_exec.c | 4 ++--
src/timezone/localtime.c | 10 +++++-----
src/timezone/strftime.c | 2 +-
src/timezone/zic.c | 4 ++--
33 files changed, 48 insertions(+), 48 deletions(-)
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 1429ac8b63..7dcca93f7b 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -1858,9 +1858,9 @@ _bt_killitems(IndexScanDesc scan)
* Mark index item as dead, if it isn't already. Since this
* happens while holding a buffer lock possibly in shared mode,
* it's possible that multiple processes attempt to do this
- * simultaneously, leading to multiple full-page images being
- * set to WAL (if wal_log_hints or data checksums are enabled),
- * which is undesirable.
+ * simultaneously, leading to multiple full-page images being set
+ * to WAL (if wal_log_hints or data checksums are enabled), which
+ * is undesirable.
*/
if (killtuple && !ItemIdIsDead(iid))
{
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 058610af9b..efd7201d61 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -753,7 +753,7 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
* further down in the plan tree.
*/
ps = queryDesc->planstate;
- if (IsA(ps, GatherState) &&((Gather *) ps->plan)->invisible)
+ if (IsA(ps, GatherState) && ((Gather *) ps->plan)->invisible)
{
ps = outerPlanState(ps);
es->hide_workers = true;
@@ -2256,7 +2256,7 @@ show_scan_qual(List *qual, const char *qlabel,
{
bool useprefix;
- useprefix = (IsA(planstate->plan, SubqueryScan) ||es->verbose);
+ useprefix = (IsA(planstate->plan, SubqueryScan) || es->verbose);
show_qual(qual, qlabel, planstate, ancestors, useprefix, es);
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8d2ed986d1..8801af589c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11136,7 +11136,7 @@ ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno)
for (;;)
{
/* only one varno, so no need to check that */
- if (IsA(expr, Var) &&((Var *) expr)->varattno == varattno)
+ if (IsA(expr, Var) && ((Var *) expr)->varattno == varattno)
return false;
else if (IsA(expr, RelabelType))
expr = (Node *) ((RelabelType *) expr)->arg;
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8891b1d564..9e5938b10e 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2176,7 +2176,7 @@ AlterDomainDefault(List *names, Node *defaultRaw)
* DefineDomain.)
*/
if (defaultExpr == NULL ||
- (IsA(defaultExpr, Const) &&((Const *) defaultExpr)->constisnull))
+ (IsA(defaultExpr, Const) && ((Const *) defaultExpr)->constisnull))
{
/* Default is NULL, drop it */
defaultExpr = NULL;
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 8553db0dd0..26111327ca 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1410,7 +1410,7 @@ find_unaggregated_cols_walker(Node *node, Bitmapset **colnos)
*colnos = bms_add_member(*colnos, var->varattno);
return false;
}
- if (IsA(node, Aggref) ||IsA(node, GroupingFunc))
+ if (IsA(node, Aggref) || IsA(node, GroupingFunc))
{
/* do not descend into aggregate exprs */
return false;
diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c
index 4a1b060fde..e8da6eaec9 100644
--- a/src/backend/executor/nodeProjectSet.c
+++ b/src/backend/executor/nodeProjectSet.c
@@ -277,8 +277,8 @@ ExecInitProjectSet(ProjectSet *node, EState *estate, int eflags)
TargetEntry *te = (TargetEntry *) lfirst(lc);
Expr *expr = te->expr;
- if ((IsA(expr, FuncExpr) &&((FuncExpr *) expr)->funcretset) ||
- (IsA(expr, OpExpr) &&((OpExpr *) expr)->opretset))
+ if ((IsA(expr, FuncExpr) && ((FuncExpr *) expr)->funcretset) ||
+ (IsA(expr, OpExpr) && ((OpExpr *) expr)->opretset))
{
state->elems[off] = (Node *)
ExecInitFunctionResultSet(expr, state->ps.ps_ExprContext,
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 7e15778da5..78a865d23b 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -80,7 +80,7 @@ static int ssl_protocol_version_to_openssl(int v);
int
be_tls_init(bool isServerStart)
{
- STACK_OF(X509_NAME) *root_cert_list = NULL;
+ STACK_OF(X509_NAME) * root_cert_list = NULL;
SSL_CTX *context;
int ssl_ver_min = -1;
int ssl_ver_max = -1;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index bb1565467d..ebf3ce37aa 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3682,7 +3682,7 @@ outNode(StringInfo str, const void *obj)
if (obj == NULL)
appendStringInfoString(str, "<>");
- else if (IsA(obj, List) ||IsA(obj, IntList) || IsA(obj, OidList))
+ else if (IsA(obj, List) || IsA(obj, IntList) || IsA(obj, OidList))
_outList(str, obj);
else if (IsA(obj, Integer) ||
IsA(obj, Float) ||
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index f4d4a4df66..b976afb69d 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -3257,7 +3257,7 @@ final_cost_mergejoin(PlannerInfo *root, MergePath *path,
* The whole issue is moot if we are working from a unique-ified outer
* input, or if we know we don't need to mark/restore at all.
*/
- if (IsA(outer_path, UniquePath) ||path->skip_mark_restore)
+ if (IsA(outer_path, UniquePath) || path->skip_mark_restore)
rescannedtuples = 0;
else
{
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 2b676bf406..baefe0e946 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1960,7 +1960,7 @@ set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset)
static void
set_param_references(PlannerInfo *root, Plan *plan)
{
- Assert(IsA(plan, Gather) ||IsA(plan, GatherMerge));
+ Assert(IsA(plan, Gather) || IsA(plan, GatherMerge));
if (plan->lefttree->extParam)
{
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index d09bc6d291..e845a4b1ae 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2639,7 +2639,7 @@ apply_projection_to_path(PlannerInfo *root,
* workers can help project. But if there is something that is not
* parallel-safe in the target expressions, then we can't.
*/
- if ((IsA(path, GatherPath) ||IsA(path, GatherMergePath)) &&
+ if ((IsA(path, GatherPath) || IsA(path, GatherMergePath)) &&
is_parallel_safe(root, (Node *) target->exprs))
{
/*
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index eee9c33637..401da5dedf 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -221,7 +221,7 @@ transformOptionalSelectInto(ParseState *pstate, Node *parseTree)
/* If it's a set-operation tree, drill down to leftmost SelectStmt */
while (stmt && stmt->op != SETOP_NONE)
stmt = stmt->larg;
- Assert(stmt && IsA(stmt, SelectStmt) &&stmt->larg == NULL);
+ Assert(stmt && IsA(stmt, SelectStmt) && stmt->larg == NULL);
if (stmt->intoClause)
{
@@ -641,7 +641,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt)
if (tle->resjunk)
continue;
if (tle->expr &&
- (IsA(tle->expr, Const) ||IsA(tle->expr, Param)) &&
+ (IsA(tle->expr, Const) || IsA(tle->expr, Param)) &&
exprType((Node *) tle->expr) == UNKNOWNOID)
expr = tle->expr;
else
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index f1cc5479e4..f813b587f1 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -750,8 +750,8 @@ check_agg_arguments_walker(Node *node,
*/
if (context->sublevels_up == 0)
{
- if ((IsA(node, FuncExpr) &&((FuncExpr *) node)->funcretset) ||
- (IsA(node, OpExpr) &&((OpExpr *) node)->opretset))
+ if ((IsA(node, FuncExpr) && ((FuncExpr *) node)->funcretset) ||
+ (IsA(node, OpExpr) && ((OpExpr *) node)->opretset))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("aggregate function calls cannot contain set-returning function calls"),
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 96ae3a7e64..6fff13479e 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -1767,7 +1767,7 @@ transformLimitClause(ParseState *pstate, Node *clause,
* unadorned NULL that's not accepted back by the grammar.
*/
if (exprKind == EXPR_KIND_LIMIT && limitOption == LIMIT_OPTION_WITH_TIES &&
- IsA(clause, A_Const) &&((A_Const *) clause)->val.type == T_Null)
+ IsA(clause, A_Const) && ((A_Const *) clause)->val.type == T_Null)
ereport(ERROR,
(errcode(ERRCODE_INVALID_ROW_COUNT_IN_LIMIT_CLAUSE),
errmsg("row count cannot be NULL in FETCH FIRST ... WITH TIES clause")));
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 831db4af95..f69976cc8c 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -961,7 +961,7 @@ transformAExprOp(ParseState *pstate, A_Expr *a)
list_length(a->name) == 1 &&
strcmp(strVal(linitial(a->name)), "=") == 0 &&
(exprIsNullConstant(lexpr) || exprIsNullConstant(rexpr)) &&
- (!IsA(lexpr, CaseTestExpr) &&!IsA(rexpr, CaseTestExpr)))
+ (!IsA(lexpr, CaseTestExpr) && !IsA(rexpr, CaseTestExpr)))
{
NullTest *n = makeNode(NullTest);
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index c7e16f2f21..ab6f1e1c9d 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -1479,13 +1479,13 @@ examine_clause_args(List *args, Var **varp, Const **cstp, bool *varonleftp)
if (IsA(rightop, RelabelType))
rightop = (Node *) ((RelabelType *) rightop)->arg;
- if (IsA(leftop, Var) &&IsA(rightop, Const))
+ if (IsA(leftop, Var) && IsA(rightop, Const))
{
var = (Var *) leftop;
cst = (Const *) rightop;
varonleft = true;
}
- else if (IsA(leftop, Const) &&IsA(rightop, Var))
+ else if (IsA(leftop, Const) && IsA(rightop, Var))
{
var = (Var *) rightop;
cst = (Const *) leftop;
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 7a965973cd..1914138a24 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -4471,7 +4471,7 @@ TemporalSimplify(int32 max_precis, Node *node)
typmod = (Node *) lsecond(expr->args);
- if (IsA(typmod, Const) &&!((Const *) typmod)->constisnull)
+ if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
{
Node *source = (Node *) linitial(expr->args);
int32 old_precis = exprTypmod(source);
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index aab5802edb..16768b28c3 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5025,7 +5025,7 @@ NUM_prepare_locale(NUMProc *Np)
if (lconv->thousands_sep && *lconv->thousands_sep)
Np->L_thousands_sep = lconv->thousands_sep;
/* Make sure thousands separator doesn't match decimal point symbol. */
- else if (strcmp(Np->decimal, ",") !=0)
+ else if (strcmp(Np->decimal, ",") != 0)
Np->L_thousands_sep = ",";
else
Np->L_thousands_sep = ".";
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 9986132b45..f3a725271e 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -911,7 +911,7 @@ numeric_support(PG_FUNCTION_ARGS)
typmod = (Node *) lsecond(expr->args);
- if (IsA(typmod, Const) &&!((Const *) typmod)->constisnull)
+ if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
{
Node *source = (Node *) linitial(expr->args);
int32 old_typmod = exprTypmod(source);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 723a8fa48c..076c3c019f 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5829,8 +5829,8 @@ get_rule_sortgroupclause(Index ref, List *tlist, bool force_colno,
*/
bool need_paren = (PRETTY_PAREN(context)
|| IsA(expr, FuncExpr)
- ||IsA(expr, Aggref)
- ||IsA(expr, WindowFunc));
+ || IsA(expr, Aggref)
+ || IsA(expr, WindowFunc));
if (need_paren)
appendStringInfoChar(context->buf, '(');
@@ -10317,7 +10317,7 @@ get_from_clause_item(Node *jtnode, Query *query, deparse_context *context)
need_paren_on_right = PRETTY_PAREN(context) &&
!IsA(j->rarg, RangeTblRef) &&
- !(IsA(j->rarg, JoinExpr) &&((JoinExpr *) j->rarg)->alias != NULL);
+ !(IsA(j->rarg, JoinExpr) && ((JoinExpr *) j->rarg)->alias != NULL);
if (!PRETTY_PAREN(context) || j->alias != NULL)
appendStringInfoChar(buf, '(');
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 4caffb5804..aa60de680d 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1263,7 +1263,7 @@ interval_support(PG_FUNCTION_ARGS)
typmod = (Node *) lsecond(expr->args);
- if (IsA(typmod, Const) &&!((Const *) typmod)->constisnull)
+ if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
{
Node *source = (Node *) linitial(expr->args);
int32 new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
diff --git a/src/backend/utils/adt/varbit.c b/src/backend/utils/adt/varbit.c
index c3257553bd..f0c6a44b84 100644
--- a/src/backend/utils/adt/varbit.c
+++ b/src/backend/utils/adt/varbit.c
@@ -713,7 +713,7 @@ varbit_support(PG_FUNCTION_ARGS)
typmod = (Node *) lsecond(expr->args);
- if (IsA(typmod, Const) &&!((Const *) typmod)->constisnull)
+ if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
{
Node *source = (Node *) linitial(expr->args);
int32 new_typmod = DatumGetInt32(((Const *) typmod)->constvalue);
diff --git a/src/backend/utils/adt/varchar.c b/src/backend/utils/adt/varchar.c
index 39acfdff6c..b595ab9569 100644
--- a/src/backend/utils/adt/varchar.c
+++ b/src/backend/utils/adt/varchar.c
@@ -572,7 +572,7 @@ varchar_support(PG_FUNCTION_ARGS)
typmod = (Node *) lsecond(expr->args);
- if (IsA(typmod, Const) &&!((Const *) typmod)->constisnull)
+ if (IsA(typmod, Const) && !((Const *) typmod)->constisnull)
{
Node *source = (Node *) linitial(expr->args);
int32 old_typmod = exprTypmod(source);
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index f178d6ac21..ac81151acc 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -396,7 +396,7 @@ _PrintFileData(ArchiveHandle *AH, char *filename)
}
free(buf);
- if (cfclose(cfp) !=0)
+ if (cfclose(cfp) != 0)
fatal("could not close data file \"%s\": %m", filename);
}
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 1e931a56cb..eb018854a5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -4191,7 +4191,7 @@ _complete_from_query(const char *simple_query,
*/
if (strcmp(schema_query->catname,
"pg_catalog.pg_class c") == 0 &&
- strncmp(text, "pg_", 3) !=0)
+ strncmp(text, "pg_", 3) != 0)
{
appendPQExpBufferStr(&query_buffer,
" AND c.relnamespace <> (SELECT oid FROM"
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index aa917d0fc9..9326f80536 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -129,7 +129,7 @@ IsValidJsonNumber(const char *str, int len)
*/
if (*str == '-')
{
- dummy_lex.input = unconstify(char *, str) +1;
+ dummy_lex.input = unconstify(char *, str) + 1;
dummy_lex.input_length = len - 1;
}
else
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 59e8e3a825..6961d7c75b 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -132,7 +132,7 @@ next_insert(char *text, int pos, bool questionmarks, bool std_strings)
for (i = p + 1; isdigit((unsigned char) text[i]); i++)
/* empty loop body */ ;
if (!isalpha((unsigned char) text[i]) &&
- isascii((unsigned char) text[i]) &&text[i] != '_')
+ isascii((unsigned char) text[i]) && text[i] != '_')
/* not dollar delimited quote */
return p;
}
diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c
index b9653c01fe..ea1146f520 100644
--- a/src/interfaces/ecpg/ecpglib/prepare.c
+++ b/src/interfaces/ecpg/ecpglib/prepare.c
@@ -132,7 +132,7 @@ replace_variables(char **text, int lineno)
for (len = 1; (*text)[ptr + len] && isvarchar((*text)[ptr + len]); len++)
/* skip */ ;
- if (!(newcopy = (char *) ecpg_alloc(strlen(*text) -len + strlen(buffer) + 1, lineno)))
+ if (!(newcopy = (char *) ecpg_alloc(strlen(*text) - len + strlen(buffer) + 1, lineno)))
{
ecpg_free(buffer);
return false;
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index df1ac209f9..34634da1ed 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -513,7 +513,7 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn,
int *names_examined,
char **first_name)
{
- STACK_OF(GENERAL_NAME) *peer_san;
+ STACK_OF(GENERAL_NAME) * peer_san;
int i;
int rc = 0;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index aeb6c8fefc..9a87cd70f1 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8164,8 +8164,8 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
if (IsA(tle_expr, Const))
break;
/* Otherwise, it had better be a Param or an outer Var */
- Assert(IsA(tle_expr, Param) ||(IsA(tle_expr, Var) &&
- ((Var *) tle_expr)->varno == OUTER_VAR));
+ Assert(IsA(tle_expr, Param) || (IsA(tle_expr, Var) &&
+ ((Var *) tle_expr)->varno == OUTER_VAR));
/* Descend to the child node */
plan = plan->lefttree;
}
diff --git a/src/timezone/localtime.c b/src/timezone/localtime.c
index 333f27300a..787f0b69d6 100644
--- a/src/timezone/localtime.c
+++ b/src/timezone/localtime.c
@@ -138,7 +138,7 @@ detzcode(const char *const codep)
* Do two's-complement negation even on non-two's-complement machines.
* If the result would be minval - 1, return minval.
*/
- result -= !TWOS_COMPLEMENT(int32) &&result != 0;
+ result -= !TWOS_COMPLEMENT(int32) && result != 0;
result += minval;
}
return result;
@@ -152,7 +152,7 @@ detzcode64(const char *const codep)
int64 one = 1;
int64 halfmaxval = one << (64 - 2);
int64 maxval = halfmaxval - 1 + halfmaxval;
- int64 minval = -TWOS_COMPLEMENT(int64) -maxval;
+ int64 minval = -TWOS_COMPLEMENT(int64) - maxval;
result = codep[0] & 0x7f;
for (i = 1; i < 8; ++i)
@@ -164,7 +164,7 @@ detzcode64(const char *const codep)
* Do two's-complement negation even on non-two's-complement machines.
* If the result would be minval - 1, return minval.
*/
- result -= !TWOS_COMPLEMENT(int64) &&result != 0;
+ result -= !TWOS_COMPLEMENT(int64) && result != 0;
result += minval;
}
return result;
@@ -173,7 +173,7 @@ detzcode64(const char *const codep)
static bool
differ_by_repeat(const pg_time_t t1, const pg_time_t t0)
{
- if (TYPE_BIT(pg_time_t) -TYPE_SIGNED(pg_time_t) <SECSPERREPEAT_BITS)
+ if (TYPE_BIT(pg_time_t) - TYPE_SIGNED(pg_time_t) < SECSPERREPEAT_BITS)
return 0;
return t1 - t0 == SECSPERREPEAT;
}
@@ -1480,7 +1480,7 @@ timesub(const pg_time_t *timep, int32 offset,
int leapdays;
tdelta = tdays / DAYSPERLYEAR;
- if (!((!TYPE_SIGNED(pg_time_t) ||INT_MIN <= tdelta)
+ if (!((!TYPE_SIGNED(pg_time_t) || INT_MIN <= tdelta)
&& tdelta <= INT_MAX))
goto out_of_range;
idelta = tdelta;
diff --git a/src/timezone/strftime.c b/src/timezone/strftime.c
index 55c617f8c2..09360db30f 100644
--- a/src/timezone/strftime.c
+++ b/src/timezone/strftime.c
@@ -499,7 +499,7 @@ _fmt(const char *format, const struct pg_tm *t, char *pt,
static char *
_conv(int n, const char *format, char *pt, const char *ptlim)
{
- char buf[INT_STRLEN_MAXIMUM(int) +1];
+ char buf[INT_STRLEN_MAXIMUM(int) + 1];
sprintf(buf, format, n);
return _add(buf, pt, ptlim);
diff --git a/src/timezone/zic.c b/src/timezone/zic.c
index c27fb456d0..9df81824a0 100644
--- a/src/timezone/zic.c
+++ b/src/timezone/zic.c
@@ -652,7 +652,7 @@ main(int argc, char **argv)
umask(umask(S_IWGRP | S_IWOTH) | (S_IWGRP | S_IWOTH));
#endif
progname = argv[0];
- if (TYPE_BIT(zic_t) <64)
+ if (TYPE_BIT(zic_t) < 64)
{
fprintf(stderr, "%s: %s\n", progname,
_("wild compilation-time specification of zic_t"));
@@ -3444,7 +3444,7 @@ yearistype(zic_t year, const char *type)
if (type == NULL || *type == '\0')
return true;
buf = emalloc(1 + 4 * strlen(yitcommand) + 2
- + INT_STRLEN_MAXIMUM(zic_t) +2 + 4 * strlen(type) + 2);
+ + INT_STRLEN_MAXIMUM(zic_t) + 2 + 4 * strlen(type) + 2);
b = shellquote(buf, yitcommand);
*b++ = ' ';
b += sprintf(b, INT64_FORMAT, year);
--
2.20.1
Thomas Munro <thomas.munro@gmail.com> writes:
Here's the patch I propose to commit to pg_bsd_indent, if the repo
lets me, and here's the result of running it on the PG tree today.
+1. I think the repo will let you in, but if not, I can do it.
regards, tom lane
On 2020-May-16, Thomas Munro wrote:
Here's the patch I propose to commit to pg_bsd_indent, if the repo
lets me, and here's the result of running it on the PG tree today.
Looks good. Of all these changes in PG, only two are of the STACK_OK()
nature, and there are 38 places that get better.
I suppose the principled way to fix that problem with STACK_OF(x)
would be to have a user-supplied list of function-like-macros that
expand to a type name, but I'm not planning to waste time on that.
+1 on just ignoring that problem as insignificant.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
On 2020-May-16, Thomas Munro wrote:
Here's the patch I propose to commit to pg_bsd_indent, if the repo
lets me, and here's the result of running it on the PG tree today.
Looks good. Of all these changes in PG, only two are of the STACK_OK()
nature, and there are 38 places that get better.
It should also be noted that there are a lot of places where we've
programmed around this silliness by artificially breaking conditions
using IsA() into multiple lines. So the "38 places" is a lowball
estimate of how much of a problem this has been.
regards, tom lane
On Sat, May 16, 2020 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
Here's the patch I propose to commit to pg_bsd_indent, if the repo
lets me, and here's the result of running it on the PG tree today.+1. I think the repo will let you in, but if not, I can do it.
It seems I cannot. Please go ahead.
I'll eventually see if I can get this into FreeBSD's usr.bin/indent.
It's possible that that process results in a request to make it
optional (some project with a lot of STACK_OF- and no IsA-style macros
wouldn't like it), but I don't think it hurts to commit it to our copy
like this in the meantime to fix our weird formatting problem.
Thomas Munro <thomas.munro@gmail.com> writes:
On Sat, May 16, 2020 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1. I think the repo will let you in, but if not, I can do it.
It seems I cannot. Please go ahead.
[ yawn... ] It's about bedtime here, but I'll take care of it in the
morning.
Off the critical path, we oughta figure out why the repo wouldn't
let you commit. What I was told was it was set up to be writable
by all PG committers.
I'll eventually see if I can get this into FreeBSD's usr.bin/indent.
+1 to that, too.
regards, tom lane
Thomas Munro <thomas.munro@gmail.com> writes:
On Sat, May 16, 2020 at 10:15 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
+1. I think the repo will let you in, but if not, I can do it.
It seems I cannot. Please go ahead.
Pushed, and I bumped pg_bsd_indent's version to 2.1.1, and synced
our core repo with that.
regards, tom lane
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
It seems I cannot. Please go ahead.
[ yawn... ] It's about bedtime here, but I'll take care of it in the
morning.Off the critical path, we oughta figure out why the repo wouldn't
let you commit. What I was told was it was set up to be writable
by all PG committers.
Just happened to see this. Might be I'm not looking at the right thing,
but from what I can tell, the repo is set up with only you as having
write access. We also don't currently have updating the pg_bsd_indent
repo on git.postgresql.org as part of our SOP for adding/removing
committers.
All of this is fixable, of course. I've CC'd this to sysadmins@ to
highlight this issue and possible change to that repo and our SOP.
Barring complaints or concerns, based on Tom's comments above, I'll
adjust that repo to be 'owned' by pginfra, with all committers having
read/write access, and add it to our committer add/remove SOP to
update that repo's access list whenever there are changes.
I'll plan to do that in a couple of days to allow for any concerns or
questions, as this isn't a critical issue at present based on the above
comments.
Thanks,
Stephen
On 16/01/2020 03.59, Thomas Munro wrote:
On Wed, Jan 15, 2020 at 11:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I just ran pgindent over some patch, and noticed that this hunk ended up
in my working tree:- if (IsA(leftop, Var) && IsA(rightop, Const)) + if (IsA(leftop, Var) &&IsA(rightop, Const))Yeah, it's been doing that for decades. I think the triggering
factor is the typedef name (Var, here) preceding the &&.It'd be nice to fix properly, but I've tended to take the path
of least resistance by breaking such lines to avoid the ugliness:if (IsA(leftop, Var) &&
IsA(rightop, Const))I am on vacation away from the Internet this week but somehow saw this
on my phone and couldn't stop myself from peeking at pg_bsd_ident
again. Yeah, "(Var)" (where Var is a known typename) causes it to
think that any following operator must be unary.One way to fix that in the cases Alvaro is referring to is to tell
override the setting so that && (and likewise ||) are never considered
to be unary, though I haven't tested this much and there are surely
other ways to achieve this:diff --git a/lexi.c b/lexi.c index d43723c..6de3227 100644 --- a/lexi.c +++ b/lexi.c @@ -655,6 +655,12 @@ stop_lit: unary_delim = state->last_u_d; break; } + + /* && and || are never unary */ + if ((token[0] == '&' && *buf_ptr == '&') || + (token[0] == '|' && *buf_ptr == '|')) + state->last_u_d = false; + while (*(e_token - 1) == *buf_ptr || *buf_ptr == '=') { /* * handle ||, &&, etc, and also things as in int *****iThe problem with that is that && sometimes *should* be formatted like
a unary operator: when it's part of the nonstandard GCC computed goto
syntax.
These comments are made in the context of pushing this change or
equivalent to FreeBSD repository.
I think this is a better approach then the one committed to
pg_bsd_indent. It's ubiquitous that the operators are binary, except -
as you mentioned - in a nonstandard GCC syntax. The alternative given
has more disadvantages, with potential impact on FreeBSD code
formatting, which it should support as well as everything else -- to a
reasonable extent. sys/kern/ is usually a decent litmus test, but I
don't claim it should show anything interesting in this particular case.
This change may seem hacky, but it would be far from the worst hack in
this program's history or even in its present form. It's actually very
much in indent's spirit, which is an attribute I neither support nor
condemn.
In any case, this change, or equivalent, should be committed to FreeBSD
repository together with a test case or two.
Piotr Stefaniak <postgres@piotr-stefaniak.me> <VE1P192MB07504EB33625F9A23F41CCD0F2B70@VE1P192MB0750.EURP192.PROD.OUTLOOK.COM> writes:
On 16/01/2020 03.59, Thomas Munro wrote:
One way to fix that in the cases Alvaro is referring to is to tell
override the setting so that && (and likewise ||) are never considered
to be unary, though I haven't tested this much and there are surely
other ways to achieve this:
I think this is a better approach then the one committed to
pg_bsd_indent. It's ubiquitous that the operators are binary, except -
as you mentioned - in a nonstandard GCC syntax. The alternative given
has more disadvantages, with potential impact on FreeBSD code
formatting, which it should support as well as everything else -- to a
reasonable extent. sys/kern/ is usually a decent litmus test, but I
don't claim it should show anything interesting in this particular case.
I think that the fix we chose is better, at least for our purposes.
You can see its effects on our source tree at
and while certainly most of the diffs are around && or || operators,
there are a fair number that are not, such as
- dummy_lex.input = unconstify(char *, str) +1;
+ dummy_lex.input = unconstify(char *, str) + 1;or more interestingly
- strncmp(text, "pg_", 3) !=0)
+ strncmp(text, "pg_", 3) != 0)where the previous misformatting is because "text" is a known typedef
name. So it appears to me that this change reduces the misformatting
cost of typedef names that chance to match field or variable names,
and that's actually quite a big issue for us. We have, ATM, 3574 known
typedefs in typedefs.list, a fair number of which are not under our
control (since they come from system headers on various platforms).
So it's inevitable that there are going to be collisions.
In short, I'm inclined to stick with the hack we've got. I'm sad that
it will result in further divergence from FreeBSD indent; but it does
useful stuff for us, and I don't want to give it up.
(That said, I don't see any penalty to carrying both changes; so we'll
probably also absorb the &&/|| change at some convenient time.)
regards, tom lane