Re: Replace remaining castNode(…, lfirst(…)) and friends calls with l*_node()

From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
To: Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Replace remaining castNode(…, lfirst(…)) and friends calls with l*_node()
Date: 2021-07-08 18:17:41
Message-ID: 202107081817.mkd7in7le6qr@alvherre.pgsql
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

+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

foreach(cell, bound_datums)
PartitionRangeDatum *datum;

datum = lfirst_node(PartitionRangeDatum, cell);
appendStringInfoString(buf, sep);

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 —
"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)

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-07-08 18:26:46 Re: Replace remaining castNode(…, lfirst(…)) and friends calls with l*_node()
Previous Message Alvaro Herrera 2021-07-08 18:15:49 Re: Pipeline mode and PQpipelineSync()