Re: Replace remaining castNode(…, lfirst(…)) and friends calls with l*_node() - Mailing list 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
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)