Replace remaining castNode(…, lfirst(…)) and friends calls with l*_node()
Hi hackers,
We've had node-casting versions of the list extraction macros since
2017, but several cases of the written-out version have been added since
then (even by Tom, who added the l*_node() macros).
Here's a patch to convert the remaining ones. The macros were
back-patched to all supported branches, so this won't create any
new back-patching hazards.
- ilmari
Attachments:
0001-Use-l-_node-family-of-functions-where-appropriate.patchtext/x-diff; charset=utf-8Download
From be3556361bcba2f332f51b75aa13946359335ae2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 7 Jul 2021 11:44:23 +0100
Subject: [PATCH] Use l*_node() family of functions where appropriate
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Instead of castNode(â¦, lfoo(â¦))
---
src/backend/commands/publicationcmds.c | 2 +-
src/backend/commands/tablecmds.c | 8 ++++----
src/backend/executor/execPartition.c | 2 +-
src/backend/executor/nodeValuesscan.c | 2 +-
src/backend/optimizer/util/paramassign.c | 2 +-
src/backend/parser/parse_clause.c | 2 +-
src/backend/parser/parse_utilcmd.c | 8 ++++----
src/backend/partitioning/partbounds.c | 18 +++++++++---------
src/backend/rewrite/rewriteSearchCycle.c | 8 ++++----
src/backend/tcop/postgres.c | 2 +-
src/backend/utils/adt/ruleutils.c | 6 +++---
src/pl/plpgsql/src/pl_exec.c | 2 +-
src/test/modules/test_predtest/test_predtest.c | 4 ++--
13 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 95c253c8e0..08f94fa324 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -515,7 +515,7 @@ OpenTableList(List *tables)
*/
foreach(lc, tables)
{
- RangeVar *rv = castNode(RangeVar, lfirst(lc));
+ RangeVar *rv = lfirst_node(RangeVar, lc);
bool recurse = rv->inh;
Relation rel;
Oid myrelid;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 97a9725df7..ba71ceed64 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -4770,7 +4770,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
foreach(lcmd, subcmds)
ATExecCmd(wqueue, tab,
- castNode(AlterTableCmd, lfirst(lcmd)),
+ lfirst_node(AlterTableCmd, lcmd),
lockmode, pass, context);
/*
@@ -12753,7 +12753,7 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
foreach(lcmd, stmt->cmds)
{
- AlterTableCmd *cmd = castNode(AlterTableCmd, lfirst(lcmd));
+ AlterTableCmd *cmd = lfirst_node(AlterTableCmd, lcmd);
if (cmd->subtype == AT_AddIndex)
{
@@ -16571,7 +16571,7 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
/* take care of any partition expressions */
foreach(l, partspec->partParams)
{
- PartitionElem *pelem = castNode(PartitionElem, lfirst(l));
+ PartitionElem *pelem = lfirst_node(PartitionElem, l);
if (pelem->expr)
{
@@ -16608,7 +16608,7 @@ ComputePartitionAttrs(ParseState *pstate, Relation rel, List *partParams, AttrNu
attn = 0;
foreach(lc, partParams)
{
- PartitionElem *pelem = castNode(PartitionElem, lfirst(lc));
+ PartitionElem *pelem = lfirst_node(PartitionElem, lc);
Oid atttype;
Oid attcollation;
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 606c920b06..5c723bc54e 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -585,7 +585,7 @@ ExecInitPartitionInfo(ModifyTableState *mtstate, EState *estate,
foreach(ll, wcoList)
{
- WithCheckOption *wco = castNode(WithCheckOption, lfirst(ll));
+ WithCheckOption *wco = lfirst_node(WithCheckOption, ll);
ExprState *wcoExpr = ExecInitQual(castNode(List, wco->qual),
&mtstate->ps);
diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 5de1429fda..00bd1271e4 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -285,7 +285,7 @@ ExecInitValuesScan(ValuesScan *node, EState *estate, int eflags)
i = 0;
foreach(vtl, node->values_lists)
{
- List *exprs = castNode(List, lfirst(vtl));
+ List *exprs = lfirst_node(List, vtl);
scanstate->exprlists[i] = exprs;
diff --git a/src/backend/optimizer/util/paramassign.c b/src/backend/optimizer/util/paramassign.c
index ebb424112b..61b856a959 100644
--- a/src/backend/optimizer/util/paramassign.c
+++ b/src/backend/optimizer/util/paramassign.c
@@ -431,7 +431,7 @@ process_subquery_nestloop_params(PlannerInfo *root, List *subplan_params)
foreach(lc, subplan_params)
{
- PlannerParamItem *pitem = castNode(PlannerParamItem, lfirst(lc));
+ PlannerParamItem *pitem = lfirst_node(PlannerParamItem, lc);
if (IsA(pitem->item, Var))
{
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 71c360bea5..b3f151d33b 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2807,7 +2807,7 @@ transformWindowDefinitions(ParseState *pstate,
(errcode(ERRCODE_WINDOWING_ERROR),
errmsg("RANGE with offset PRECEDING/FOLLOWING requires exactly one ORDER BY column"),
parser_errposition(pstate, windef->location)));
- sortcl = castNode(SortGroupClause, linitial(wc->orderClause));
+ sortcl = linitial_node(SortGroupClause, wc->orderClause);
sortkey = get_sortgroupclause_expr(sortcl, *targetlist);
/* Find the sort operator in pg_amop */
if (!get_ordering_op_properties(sortcl->sortop,
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 81d3e7990c..16abacd76c 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -2425,7 +2425,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
/* Make sure referenced column exists. */
foreach(columns, cxt->columns)
{
- column = castNode(ColumnDef, lfirst(columns));
+ column = lfirst_node(ColumnDef, columns);
if (strcmp(column->colname, key) == 0)
{
found = true;
@@ -2463,7 +2463,7 @@ transformIndexConstraint(Constraint *constraint, CreateStmtContext *cxt)
foreach(inher, cxt->inhRelations)
{
- RangeVar *inh = castNode(RangeVar, lfirst(inher));
+ RangeVar *inh = lfirst_node(RangeVar, inher);
Relation rel;
int count;
@@ -4089,7 +4089,7 @@ transformPartitionBound(ParseState *pstate, Relation parent,
duplicate = false;
foreach(cell2, result_spec->listdatums)
{
- Const *value2 = castNode(Const, lfirst(cell2));
+ Const *value2 = lfirst_node(Const, cell2);
if (equal(value, value2))
{
@@ -4268,7 +4268,7 @@ validateInfiniteBounds(ParseState *pstate, List *blist)
foreach(lc, blist)
{
- PartitionRangeDatum *prd = castNode(PartitionRangeDatum, lfirst(lc));
+ PartitionRangeDatum *prd = lfirst_node(PartitionRangeDatum, lc);
if (kind == prd->kind)
continue;
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 38baaefcda..be35068384 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -453,7 +453,7 @@ get_non_null_list_datum_count(PartitionBoundSpec **boundspecs, int nparts)
foreach(lc, boundspecs[i]->listdatums)
{
- Const *val = castNode(Const, lfirst(lc));
+ Const *val = lfirst_node(Const, lc);
if (!val->constisnull)
count++;
@@ -514,7 +514,7 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
foreach(c, spec->listdatums)
{
- Const *val = castNode(Const, lfirst(c));
+ Const *val = lfirst_node(Const, c);
if (!val->constisnull)
{
@@ -3015,7 +3015,7 @@ check_new_partition_bound(char *relname, Relation parent,
foreach(cell, spec->listdatums)
{
- Const *val = castNode(Const, lfirst(cell));
+ Const *val = lfirst_node(Const, cell);
overlap_location = val->location;
if (!val->constisnull)
@@ -3400,7 +3400,7 @@ make_one_partition_rbound(PartitionKey key, int index, List *datums, bool lower)
i = 0;
foreach(lc, datums)
{
- PartitionRangeDatum *datum = castNode(PartitionRangeDatum, lfirst(lc));
+ PartitionRangeDatum *datum = lfirst_node(PartitionRangeDatum, lc);
/* What's contained in this range datum? */
bound->kind[i] = datum->kind;
@@ -4104,7 +4104,7 @@ get_qual_for_list(Relation parent, PartitionBoundSpec *spec)
*/
foreach(cell, spec->listdatums)
{
- Const *val = castNode(Const, lfirst(cell));
+ Const *val = lfirst_node(Const, cell);
if (val->constisnull)
list_has_null = true;
@@ -4359,8 +4359,8 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
Datum test_result;
bool isNull;
- ldatum = castNode(PartitionRangeDatum, lfirst(cell1));
- udatum = castNode(PartitionRangeDatum, lfirst(cell2));
+ ldatum = lfirst_node(PartitionRangeDatum, cell1);
+ udatum = lfirst_node(PartitionRangeDatum, cell2);
/*
* Since get_range_key_properties() modifies partexprs_item, and we
@@ -4441,11 +4441,11 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
PartitionRangeDatum *ldatum_next = NULL,
*udatum_next = NULL;
- ldatum = castNode(PartitionRangeDatum, lfirst(cell1));
+ ldatum = lfirst_node(PartitionRangeDatum, cell1);
if (lnext(spec->lowerdatums, cell1))
ldatum_next = castNode(PartitionRangeDatum,
lfirst(lnext(spec->lowerdatums, cell1)));
- udatum = castNode(PartitionRangeDatum, lfirst(cell2));
+ udatum = lfirst_node(PartitionRangeDatum, cell2);
if (lnext(spec->upperdatums, cell2))
udatum_next = castNode(PartitionRangeDatum,
lfirst(lnext(spec->upperdatums, cell2)));
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c
index 599fe8e735..c50ebdba24 100644
--- a/src/backend/rewrite/rewriteSearchCycle.c
+++ b/src/backend/rewrite/rewriteSearchCycle.c
@@ -307,8 +307,8 @@ rewriteSearchAndCycle(CommonTableExpr *cte)
list_nth_oid(cte->ctecolcollations, i),
0);
tle = makeTargetEntry((Expr *) var, i + 1, strVal(list_nth(cte->ctecolnames, i)), false);
- tle->resorigtbl = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigtbl;
- tle->resorigcol = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigcol;
+ tle->resorigtbl = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigtbl;
+ tle->resorigcol = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigcol;
newq1->targetList = lappend(newq1->targetList, tle);
}
@@ -482,8 +482,8 @@ rewriteSearchAndCycle(CommonTableExpr *cte)
list_nth_oid(cte->ctecolcollations, i),
0);
tle = makeTargetEntry((Expr *) var, i + 1, strVal(list_nth(cte->ctecolnames, i)), false);
- tle->resorigtbl = castNode(TargetEntry, list_nth(rte2->subquery->targetList, i))->resorigtbl;
- tle->resorigcol = castNode(TargetEntry, list_nth(rte2->subquery->targetList, i))->resorigcol;
+ tle->resorigtbl = list_nth_node(TargetEntry, rte2->subquery->targetList, i)->resorigtbl;
+ tle->resorigcol = list_nth_node(TargetEntry, rte2->subquery->targetList, i)->resorigcol;
newq2->targetList = lappend(newq2->targetList, tle);
}
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 8cea10c901..530caa520b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -785,7 +785,7 @@ pg_rewrite_query(Query *query)
*/
foreach(lc, querytree_list)
{
- Query *query = castNode(Query, lfirst(lc));
+ Query *query = lfirst_node(Query, lc);
if (query->commandType != CMD_UTILITY)
{
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3719755a0d..5e7108f903 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -7793,7 +7793,7 @@ find_param_referent(Param *param, deparse_context *context,
*/
foreach(lc2, ((Plan *) ancestor)->initPlan)
{
- SubPlan *subplan = castNode(SubPlan, lfirst(lc2));
+ SubPlan *subplan = lfirst_node(SubPlan, lc2);
if (child_plan != (Plan *) list_nth(dpns->subplans,
subplan->plan_id - 1))
@@ -9407,7 +9407,7 @@ get_rule_expr(Node *node, deparse_context *context,
sep = "";
foreach(cell, spec->listdatums)
{
- Const *val = castNode(Const, lfirst(cell));
+ Const *val = lfirst_node(Const, cell);
appendStringInfoString(buf, sep);
get_const_expr(val, context, -1);
@@ -11990,7 +11990,7 @@ get_range_partbound_string(List *bound_datums)
foreach(cell, bound_datums)
{
PartitionRangeDatum *datum =
- castNode(PartitionRangeDatum, lfirst(cell));
+ lfirst_node(PartitionRangeDatum, cell);
appendStringInfoString(buf, sep);
if (datum->kind == PARTITION_RANGE_DATUM_MINVALUE)
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 96bb77e0b1..b5c208d83d 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -7996,7 +7996,7 @@ exec_save_simple_expr(PLpgSQL_expr *expr, CachedPlan *cplan)
{
/* Extract the single tlist expression */
Assert(list_length(plan->targetlist) == 1);
- tle_expr = castNode(TargetEntry, linitial(plan->targetlist))->expr;
+ tle_expr = linitial_node(TargetEntry, plan->targetlist)->expr;
if (IsA(plan, Result))
{
diff --git a/src/test/modules/test_predtest/test_predtest.c b/src/test/modules/test_predtest/test_predtest.c
index 9c0aadd61c..0301d48443 100644
--- a/src/test/modules/test_predtest/test_predtest.c
+++ b/src/test/modules/test_predtest/test_predtest.c
@@ -131,8 +131,8 @@ test_predtest(PG_FUNCTION_ARGS)
elog(ERROR, "failed to decipher query plan");
plan = stmt->planTree;
Assert(list_length(plan->targetlist) >= 2);
- clause1 = castNode(TargetEntry, linitial(plan->targetlist))->expr;
- clause2 = castNode(TargetEntry, lsecond(plan->targetlist))->expr;
+ clause1 = linitial_node(TargetEntry, plan->targetlist)->expr;
+ clause2 = lsecond_node(TargetEntry, plan->targetlist)->expr;
/*
* Because the clauses are in the SELECT list, preprocess_expression did
--
2.30.2
On 7 Jul 2021, at 21:12, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
Here's a patch to convert the remaining ones.
I haven't tested it yet, but +1 on the idea of cleaning these up making the
codebase consistent.
--
Daniel Gustafsson https://vmware.com/
Daniel Gustafsson <daniel@yesql.se> writes:
On 7 Jul 2021, at 21:12, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
Here's a patch to convert the remaining ones.
I haven't tested it yet, but +1 on the idea of cleaning these up making the
codebase consistent.
FWIW, it passes `make check-world` on an assert- and TAP-enabled Linux build.
- ilmari
+1 on the idea. On a quick glance, all changes looks good.
On 2021-Jul-07, Dagfinn Ilmari Mannsåker wrote:
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c index 599fe8e735..c50ebdba24 100644 --- a/src/backend/rewrite/rewriteSearchCycle.c +++ b/src/backend/rewrite/rewriteSearchCycle.c @@ -307,8 +307,8 @@ rewriteSearchAndCycle(CommonTableExpr *cte) list_nth_oid(cte->ctecolcollations, i), 0); tle = makeTargetEntry((Expr *) var, i + 1, strVal(list_nth(cte->ctecolnames, i)), false); - tle->resorigtbl = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigtbl; - tle->resorigcol = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigcol; + tle->resorigtbl = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigtbl; + tle->resorigcol = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigcol;
This seems a bit surprising to me. I mean, clearly we trust our List
implementation to be so good that we can just fetch the same node twice,
one for each member of the same struct, and the compiler will optimize
everything so that it's a single access to the n'th list entry. Is this
true? I would have expected there to be a single fetch of the struct,
followed by an access of each of the two struct members.
@@ -11990,7 +11990,7 @@ get_range_partbound_string(List *bound_datums) foreach(cell, bound_datums) { PartitionRangeDatum *datum = - castNode(PartitionRangeDatum, lfirst(cell)); + lfirst_node(PartitionRangeDatum, cell);
This is pretty personal and subjective, but stylistically I dislike
initializations that indent to the same level as the variable
declarations they follow; they still require a second line of code and
don't look good when in between other declarations. The style is at
odds with what pgindent does when the assignment is not an
initialization. We seldom use this style. In this particular case it
is possible to split more cleanly while ending up with exactly the same
line count by removing the initialization and making it a straight
assignment,
foreach(cell, bound_datums)
{
PartitionRangeDatum *datum;
datum = lfirst_node(PartitionRangeDatum, cell);
appendStringInfoString(buf, sep);
if (datum->kind == PARTITION_RANGE_DATUM_MINVALUE)
This doesn't bother me enough to change on its own, but if we're
modifying the same line here, we may as well clean this up.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"No deja de ser humillante para una persona de ingenio saber
que no hay tonto que no le pueda enseñar algo." (Jean B. Say)
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
On 2021-Jul-07, Dagfinn Ilmari Mannsåker wrote:
PartitionRangeDatum *datum = - castNode(PartitionRangeDatum, lfirst(cell)); + lfirst_node(PartitionRangeDatum, cell);
This is pretty personal and subjective, but stylistically I dislike
initializations that indent to the same level as the variable
declarations they follow; they still require a second line of code and
don't look good when in between other declarations.
Yeah, this seems like a pgindent bug to me, but I've not mustered the
energy to try to fix it. As you say, it can be worked around by not
trying to lay out the code that way.
regards, tom lane
On 08.07.21 20:17, Alvaro Herrera wrote:
diff --git a/src/backend/rewrite/rewriteSearchCycle.c b/src/backend/rewrite/rewriteSearchCycle.c index 599fe8e735..c50ebdba24 100644 --- a/src/backend/rewrite/rewriteSearchCycle.c +++ b/src/backend/rewrite/rewriteSearchCycle.c @@ -307,8 +307,8 @@ rewriteSearchAndCycle(CommonTableExpr *cte) list_nth_oid(cte->ctecolcollations, i), 0); tle = makeTargetEntry((Expr *) var, i + 1, strVal(list_nth(cte->ctecolnames, i)), false); - tle->resorigtbl = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigtbl; - tle->resorigcol = castNode(TargetEntry, list_nth(rte1->subquery->targetList, i))->resorigcol; + tle->resorigtbl = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigtbl; + tle->resorigcol = list_nth_node(TargetEntry, rte1->subquery->targetList, i)->resorigcol;This seems a bit surprising to me. I mean, clearly we trust our List
implementation to be so good that we can just fetch the same node twice,
one for each member of the same struct, and the compiler will optimize
everything so that it's a single access to the n'th list entry. Is this
true? I would have expected there to be a single fetch of the struct,
followed by an access of each of the two struct members.
Lists are arrays now internally, so accessing an element by number is
pretty cheap.
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
Hi hackers,
We've had node-casting versions of the list extraction macros since
2017, but several cases of the written-out version have been added since
then (even by Tom, who added the l*_node() macros).Here's a patch to convert the remaining ones. The macros were
back-patched to all supported branches, so this won't create any
new back-patching hazards.
Added to the 2021-09 commit fest: https://commitfest.postgresql.org/34/3253/
- ilmari
On 15.07.21 13:12, Dagfinn Ilmari Mannsåker wrote:
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
Hi hackers,
We've had node-casting versions of the list extraction macros since
2017, but several cases of the written-out version have been added since
then (even by Tom, who added the l*_node() macros).Here's a patch to convert the remaining ones. The macros were
back-patched to all supported branches, so this won't create any
new back-patching hazards.Added to the 2021-09 commit fest: https://commitfest.postgresql.org/34/3253/
committed
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 15.07.21 13:12, Dagfinn Ilmari Mannsåker wrote:
ilmari@ilmari.org (Dagfinn Ilmari Mannsåker) writes:
Hi hackers,
We've had node-casting versions of the list extraction macros since
2017, but several cases of the written-out version have been added since
then (even by Tom, who added the l*_node() macros).Here's a patch to convert the remaining ones. The macros were
back-patched to all supported branches, so this won't create any
new back-patching hazards.Added to the 2021-09 commit fest: https://commitfest.postgresql.org/34/3253/
committed
Thanks!
- ilmari