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

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