Re: POC: converting Lists into arrays - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: POC: converting Lists into arrays |
Date | |
Msg-id | 9410.1565289558@sss.pgh.pa.us Whole thread Raw |
In response to | Re: POC: converting Lists into arrays (Andres Freund <andres@anarazel.de>) |
List | pgsql-hackers |
Andres Freund <andres@anarazel.de> writes: > On 2019-07-31 19:40:09 -0400, Tom Lane wrote: >> That makes the other idea (of a foreach-ish macro declaring the >> listcell value variable) problematic, too :-(. > Hm. One way partially around that would be using an anonymous struct > inside the for(). Something like > #define foreach_node(membertype, name, lst) \ > for (struct {membertype *node; ListCell *lc; const List *l; int i;} name = {...}; \ > ...) > which then would allow code like > foreach_node(OpExpr, cur, list) > { > do_something_with_node(cur.node); > foreach_delete_current(cur); > } I'm hesitant to change the look of our loops quite that much, mainly because it'll be a pain for back-patching. If you write some code for HEAD like this, and then have to back-patch it, you'll need to insert/change significantly more code than if it's just a matter of whether there's a ListCell variable or not. I experimented with the "aforeach" idea I suggested upthread, to the extent of writing the macros and then converting parse_clause.c (a file chosen more or less at random) to use aforeach instead of foreach. I was somewhat surprised to find that every single foreach() did convert pleasantly. (There are several forboth's that I didn't try to do anything with, though.) If we do go in this direction, I wouldn't suggest trying to actually do wholesale conversion of existing code like this; that seems more likely to create back-patching land mines than do anything helpful. I am slightly tempted to try to convert everyplace using foreach_delete_current, though, since those loops are different from v12 already. Thoughts? regards, tom lane diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index 2a6b2ff..39d8d8e 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -117,8 +117,6 @@ static Node *transformFrameOffset(ParseState *pstate, int frameOptions, void transformFromClause(ParseState *pstate, List *frmList) { - ListCell *fl; - /* * The grammar will have produced a list of RangeVars, RangeSubselects, * RangeFunctions, and/or JoinExprs. Transform each one (possibly adding @@ -128,9 +126,9 @@ transformFromClause(ParseState *pstate, List *frmList) * Note we must process the items left-to-right for proper handling of * LATERAL references. */ - foreach(fl, frmList) + aforeach(frmList) { - Node *n = lfirst(fl); + Node *n = (Node *) aforeach_current(); RangeTblEntry *rte; int rtindex; List *namespace; @@ -267,11 +265,10 @@ extractRemainingColumns(List *common_colnames, { char *colname = strVal(lfirst(lnames)); bool match = false; - ListCell *cnames; - foreach(cnames, common_colnames) + aforeach(common_colnames) { - char *ccolname = strVal(lfirst(cnames)); + char *ccolname = strVal(aforeach_current()); if (strcmp(colname, ccolname) == 0) { @@ -475,7 +472,6 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r) List *coldeflists = NIL; bool is_lateral; RangeTblEntry *rte; - ListCell *lc; /* * We make lateral_only names of this level visible, whether or not the @@ -505,9 +501,9 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r) * Likewise, collect column definition lists if there were any. But * complain if we find one here and the RangeFunction has one too. */ - foreach(lc, r->functions) + aforeach(r->functions) { - List *pair = (List *) lfirst(lc); + List *pair = aforeach_current_node(List); Node *fexpr; List *coldeflist; Node *newfexpr; @@ -551,11 +547,9 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r) fc->over == NULL && coldeflist == NIL) { - ListCell *lc; - - foreach(lc, fc->args) + aforeach(fc->args) { - Node *arg = (Node *) lfirst(lc); + Node *arg = (Node *) aforeach_current(); FuncCall *newfc; last_srf = pstate->p_last_srf; @@ -700,7 +694,6 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) Oid docType; RangeTblEntry *rte; bool is_lateral; - ListCell *col; char **names; int colno; @@ -743,9 +736,9 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) names = palloc(sizeof(char *) * list_length(rtf->columns)); colno = 0; - foreach(col, rtf->columns) + aforeach(rtf->columns) { - RangeTableFuncCol *rawc = (RangeTableFuncCol *) lfirst(col); + RangeTableFuncCol *rawc = aforeach_current_node(RangeTableFuncCol); Oid typid; int32 typmod; Node *colexpr; @@ -837,15 +830,13 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) /* Namespaces, if any, also need to be transformed */ if (rtf->namespaces != NIL) { - ListCell *ns; - ListCell *lc2; List *ns_uris = NIL; List *ns_names = NIL; bool default_ns_seen = false; - foreach(ns, rtf->namespaces) + aforeach(rtf->namespaces) { - ResTarget *r = (ResTarget *) lfirst(ns); + ResTarget *r = aforeach_current_node(ResTarget); Node *ns_uri; Assert(IsA(r, ResTarget)); @@ -858,9 +849,9 @@ transformRangeTableFunc(ParseState *pstate, RangeTableFunc *rtf) /* Verify consistency of name list: no dupes, only one DEFAULT */ if (r->name != NULL) { - foreach(lc2, ns_names) + aforeach(ns_names) { - Value *ns_node = (Value *) lfirst(lc2); + Value *ns_node = (Value *) aforeach_current(); if (ns_node == NULL) continue; @@ -1268,19 +1259,17 @@ transformFromClauseItem(ParseState *pstate, Node *n, if (j->isNatural) { List *rlist = NIL; - ListCell *lx, - *rx; Assert(j->usingClause == NIL); /* shouldn't have USING() too */ - foreach(lx, l_colnames) + aforeach(l_colnames) { - char *l_colname = strVal(lfirst(lx)); + char *l_colname = strVal(aforeach_current()); Value *m_name = NULL; - foreach(rx, r_colnames) + aforeach(r_colnames) { - char *r_colname = strVal(lfirst(rx)); + char *r_colname = strVal(aforeach_current()); if (strcmp(l_colname, r_colname) == 0) { @@ -1313,24 +1302,21 @@ transformFromClauseItem(ParseState *pstate, Node *n, List *ucols = j->usingClause; List *l_usingvars = NIL; List *r_usingvars = NIL; - ListCell *ucol; Assert(j->quals == NULL); /* shouldn't have ON() too */ - foreach(ucol, ucols) + aforeach(ucols) { - char *u_colname = strVal(lfirst(ucol)); - ListCell *col; - int ndx; + char *u_colname = strVal(aforeach_current()); int l_index = -1; int r_index = -1; Var *l_colvar, *r_colvar; /* Check for USING(foo,foo) */ - foreach(col, res_colnames) + aforeach(res_colnames) { - char *res_colname = strVal(lfirst(col)); + char *res_colname = strVal(aforeach_current()); if (strcmp(res_colname, u_colname) == 0) ereport(ERROR, @@ -1340,10 +1326,9 @@ transformFromClauseItem(ParseState *pstate, Node *n, } /* Find it in left input */ - ndx = 0; - foreach(col, l_colnames) + aforeach(l_colnames) { - char *l_colname = strVal(lfirst(col)); + char *l_colname = strVal(aforeach_current()); if (strcmp(l_colname, u_colname) == 0) { @@ -1352,9 +1337,8 @@ transformFromClauseItem(ParseState *pstate, Node *n, (errcode(ERRCODE_AMBIGUOUS_COLUMN), errmsg("common column name \"%s\" appears more than once in left table", u_colname))); - l_index = ndx; + l_index = aforeach_current_index(); } - ndx++; } if (l_index < 0) ereport(ERROR, @@ -1363,10 +1347,9 @@ transformFromClauseItem(ParseState *pstate, Node *n, u_colname))); /* Find it in right input */ - ndx = 0; - foreach(col, r_colnames) + aforeach(r_colnames) { - char *r_colname = strVal(lfirst(col)); + char *r_colname = strVal(aforeach_current()); if (strcmp(r_colname, u_colname) == 0) { @@ -1375,9 +1358,8 @@ transformFromClauseItem(ParseState *pstate, Node *n, (errcode(ERRCODE_AMBIGUOUS_COLUMN), errmsg("common column name \"%s\" appears more than once in right table", u_colname))); - r_index = ndx; + r_index = aforeach_current_index(); } - ndx++; } if (r_index < 0) ereport(ERROR, @@ -1390,7 +1372,7 @@ transformFromClauseItem(ParseState *pstate, Node *n, r_colvar = list_nth(r_colvars, r_index); r_usingvars = lappend(r_usingvars, r_colvar); - res_colnames = lappend(res_colnames, lfirst(ucol)); + res_colnames = lappend(res_colnames, aforeach_current()); res_colvars = lappend(res_colvars, buildMergedJoinVar(pstate, j->jointype, @@ -1643,11 +1625,9 @@ makeNamespaceItem(RangeTblEntry *rte, bool rel_visible, bool cols_visible, static void setNamespaceColumnVisibility(List *namespace, bool cols_visible) { - ListCell *lc; - - foreach(lc, namespace) + aforeach(namespace) { - ParseNamespaceItem *nsitem = (ParseNamespaceItem *) lfirst(lc); + ParseNamespaceItem *nsitem = (ParseNamespaceItem *) aforeach_current(); nsitem->p_cols_visible = cols_visible; } @@ -1660,11 +1640,9 @@ setNamespaceColumnVisibility(List *namespace, bool cols_visible) static void setNamespaceLateralState(List *namespace, bool lateral_only, bool lateral_ok) { - ListCell *lc; - - foreach(lc, namespace) + aforeach(namespace) { - ParseNamespaceItem *nsitem = (ParseNamespaceItem *) lfirst(lc); + ParseNamespaceItem *nsitem = (ParseNamespaceItem *) aforeach_current(); nsitem->p_lateral_only = lateral_only; nsitem->p_lateral_ok = lateral_ok; @@ -1822,8 +1800,6 @@ static TargetEntry * findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, ParseExprKind exprKind) { - ListCell *tl; - /*---------- * Handle two special cases as mandated by the SQL92 spec: * @@ -1895,9 +1871,9 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, { TargetEntry *target_result = NULL; - foreach(tl, *tlist) + aforeach(*tlist) { - TargetEntry *tle = (TargetEntry *) lfirst(tl); + TargetEntry *tle = aforeach_current_node(TargetEntry); if (!tle->resjunk && strcmp(tle->resname, name) == 0) @@ -1944,9 +1920,9 @@ findTargetlistEntrySQL92(ParseState *pstate, Node *node, List **tlist, parser_errposition(pstate, location))); target_pos = intVal(val); - foreach(tl, *tlist) + aforeach(*tlist) { - TargetEntry *tle = (TargetEntry *) lfirst(tl); + TargetEntry *tle = aforeach_current_node(TargetEntry); if (!tle->resjunk) { @@ -1990,7 +1966,6 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist, ParseExprKind exprKind) { TargetEntry *target_result; - ListCell *tl; Node *expr; /* @@ -2002,9 +1977,9 @@ findTargetlistEntrySQL99(ParseState *pstate, Node *node, List **tlist, */ expr = transformExpr(pstate, node, exprKind); - foreach(tl, *tlist) + aforeach(*tlist) { - TargetEntry *tle = (TargetEntry *) lfirst(tl); + TargetEntry *tle = aforeach_current_node(TargetEntry); Node *texpr; /* @@ -2094,7 +2069,6 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets) case T_GroupingSet: { GroupingSet *gset = (GroupingSet *) expr; - ListCell *l2; List *result_set = NIL; if (hasGroupingSets) @@ -2109,9 +2083,9 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets) if (toplevel && gset->kind == GROUPING_SET_EMPTY) return (Node *) NIL; - foreach(l2, gset->content) + aforeach(gset->content) { - Node *n1 = lfirst(l2); + Node *n1 = (Node *) aforeach_current(); Node *n2 = flatten_grouping_sets(n1, false, NULL); if (IsA(n1, GroupingSet) && @@ -2139,13 +2113,13 @@ flatten_grouping_sets(Node *expr, bool toplevel, bool *hasGroupingSets) case T_List: { List *result = NIL; - ListCell *l; - foreach(l, (List *) expr) + aforeach((List *) expr) { - Node *n = flatten_grouping_sets(lfirst(l), toplevel, hasGroupingSets); + Node *n = (Node *) aforeach_current(); - if (n != (Node *) NIL) + n = flatten_grouping_sets(n, toplevel, hasGroupingSets); + if (n) { if (IsA(n, List)) result = list_concat(result, (List *) n); @@ -2200,8 +2174,6 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local, if (tle->ressortgroupref > 0) { - ListCell *sl; - /* * Eliminate duplicates (GROUP BY x, x) but only at local level. * (Duplicates in grouping sets can affect the number of returned @@ -2239,10 +2211,9 @@ transformGroupClauseExpr(List **flatresult, Bitmapset *seen_local, * another sort step is going to be inevitable, but that's the * planner's problem. */ - - foreach(sl, sortClause) + aforeach(sortClause) { - SortGroupClause *sc = (SortGroupClause *) lfirst(sl); + SortGroupClause *sc = aforeach_current_node(SortGroupClause); if (sc->tleSortGroupRef == tle->ressortgroupref) { @@ -2298,11 +2269,10 @@ transformGroupClauseList(List **flatresult, { Bitmapset *seen_local = NULL; List *result = NIL; - ListCell *gl; - foreach(gl, list) + aforeach(list) { - Node *gexpr = (Node *) lfirst(gl); + Node *gexpr = (Node *) aforeach_current(); Index ref = transformGroupClauseExpr(flatresult, seen_local, @@ -2349,14 +2319,13 @@ transformGroupingSet(List **flatresult, List **targetlist, List *sortClause, ParseExprKind exprKind, bool useSQL99, bool toplevel) { - ListCell *gl; List *content = NIL; Assert(toplevel || gset->kind != GROUPING_SET_SETS); - foreach(gl, gset->content) + aforeach(gset->content) { - Node *n = lfirst(gl); + Node *n = (Node *) aforeach_current(); if (IsA(n, List)) { @@ -2371,7 +2340,7 @@ transformGroupingSet(List **flatresult, } else if (IsA(n, GroupingSet)) { - GroupingSet *gset2 = (GroupingSet *) lfirst(gl); + GroupingSet *gset2 = (GroupingSet *) n; content = lappend(content, transformGroupingSet(flatresult, pstate, gset2, @@ -2455,7 +2424,6 @@ transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets, List *result = NIL; List *flat_grouplist; List *gsets = NIL; - ListCell *gl; bool hasGroupingSets = false; Bitmapset *seen_local = NULL; @@ -2481,9 +2449,9 @@ transformGroupClause(ParseState *pstate, List *grouplist, List **groupingSets, exprLocation((Node *) grouplist))); } - foreach(gl, flat_grouplist) + aforeach(flat_grouplist) { - Node *gexpr = (Node *) lfirst(gl); + Node *gexpr = (Node *) aforeach_current(); if (IsA(gexpr, GroupingSet)) { @@ -2555,11 +2523,10 @@ transformSortClause(ParseState *pstate, bool useSQL99) { List *sortlist = NIL; - ListCell *olitem; - foreach(olitem, orderlist) + aforeach(orderlist) { - SortBy *sortby = (SortBy *) lfirst(olitem); + SortBy *sortby = aforeach_current_node(SortBy); TargetEntry *tle; if (useSQL99) @@ -2587,11 +2554,10 @@ transformWindowDefinitions(ParseState *pstate, { List *result = NIL; Index winref = 0; - ListCell *lc; - foreach(lc, windowdefs) + aforeach(windowdefs) { - WindowDef *windef = (WindowDef *) lfirst(lc); + WindowDef *windef = aforeach_current_node(WindowDef); WindowClause *refwc = NULL; List *partitionClause; List *orderClause; @@ -2805,8 +2771,6 @@ transformDistinctClause(ParseState *pstate, List **targetlist, List *sortClause, bool is_agg) { List *result = NIL; - ListCell *slitem; - ListCell *tlitem; /* * The distinctClause should consist of all ORDER BY items followed by all @@ -2823,9 +2787,9 @@ transformDistinctClause(ParseState *pstate, * effect will be that the TLE value will be made unique according to both * sortops. */ - foreach(slitem, sortClause) + aforeach(sortClause) { - SortGroupClause *scl = (SortGroupClause *) lfirst(slitem); + SortGroupClause *scl = aforeach_current_node(SortGroupClause); TargetEntry *tle = get_sortgroupclause_tle(scl, *targetlist); if (tle->resjunk) @@ -2843,9 +2807,9 @@ transformDistinctClause(ParseState *pstate, * Now add any remaining non-resjunk tlist items, using default sort/group * semantics for their data types. */ - foreach(tlitem, *targetlist) + aforeach(*targetlist) { - TargetEntry *tle = (TargetEntry *) lfirst(tlitem); + TargetEntry *tle = aforeach_current_node(TargetEntry); if (tle->resjunk) continue; /* ignore junk */ @@ -2902,9 +2866,9 @@ transformDistinctOnClause(ParseState *pstate, List *distinctlist, * Also notice that we could have duplicate DISTINCT ON expressions and * hence duplicate entries in sortgrouprefs.) */ - foreach(lc, distinctlist) + aforeach(distinctlist) { - Node *dexpr = (Node *) lfirst(lc); + Node *dexpr = (Node *) aforeach_current(); int sortgroupref; TargetEntry *tle; @@ -2923,9 +2887,9 @@ transformDistinctOnClause(ParseState *pstate, List *distinctlist, * in DISTINCT ON. */ skipped_sortitem = false; - foreach(lc, sortClause) + aforeach(sortClause) { - SortGroupClause *scl = (SortGroupClause *) lfirst(lc); + SortGroupClause *scl = aforeach_current_node(SortGroupClause); if (list_member_int(sortgrouprefs, scl->tleSortGroupRef)) { @@ -3021,11 +2985,10 @@ resolve_unique_index_expr(ParseState *pstate, InferClause *infer, Relation heapRel) { List *result = NIL; - ListCell *l; - foreach(l, infer->indexElems) + aforeach(infer->indexElems) { - IndexElem *ielem = (IndexElem *) lfirst(l); + IndexElem *ielem = aforeach_current_node(IndexElem); InferenceElem *pInfer = makeNode(InferenceElem); Node *parse; @@ -3422,16 +3385,15 @@ Index assignSortGroupRef(TargetEntry *tle, List *tlist) { Index maxRef; - ListCell *l; if (tle->ressortgroupref) /* already has one? */ return tle->ressortgroupref; /* easiest way to pick an unused refnumber: max used + 1 */ maxRef = 0; - foreach(l, tlist) + aforeach(tlist) { - Index ref = ((TargetEntry *) lfirst(l))->ressortgroupref; + Index ref = aforeach_current_node(TargetEntry)->ressortgroupref; if (ref > maxRef) maxRef = ref; @@ -3463,15 +3425,14 @@ bool targetIsInSortList(TargetEntry *tle, Oid sortop, List *sortList) { Index ref = tle->ressortgroupref; - ListCell *l; /* no need to scan list if tle has no marker */ if (ref == 0) return false; - foreach(l, sortList) + aforeach(sortList) { - SortGroupClause *scl = (SortGroupClause *) lfirst(l); + SortGroupClause *scl = aforeach_current_node(SortGroupClause); if (scl->tleSortGroupRef == ref && (sortop == InvalidOid || @@ -3489,11 +3450,9 @@ targetIsInSortList(TargetEntry *tle, Oid sortop, List *sortList) static WindowClause * findWindowClause(List *wclist, const char *name) { - ListCell *l; - - foreach(l, wclist) + aforeach(wclist) { - WindowClause *wc = (WindowClause *) lfirst(l); + WindowClause *wc = aforeach_current_node(WindowClause); if (wc->name && strcmp(wc->name, name) == 0) return wc; diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 1463408..21d0a67 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -381,6 +381,59 @@ lnext(const List *l, const ListCell *c) #define foreach_current_index(cell) (cell##__state.i) /* + * aforeach (anonymous foreach) - + * another convenience macro for looping through a list + * + * This is the same as foreach() except that no ListCell variable is needed. + * Instead, reference the current list element with the appropriate variant + * of aforeach_current(). + */ +#define aforeach(lst) \ + for (ForEachState aforeach__state = {(lst), 0}; \ + aforeach__state.l != NIL && \ + aforeach__state.i < aforeach__state.l->length; \ + aforeach__state.i++) + +/* + * aforeach_current... - + * + * Access the current element of the most closely nested aforeach() loop. + * These exist in the same variants that lfirst...() has. + */ +#define aforeach_current() \ + (aforeach__state.l->elements[aforeach__state.i].ptr_value) +#define aforeach_current_int() \ + (aforeach__state.l->elements[aforeach__state.i].int_value) +#define aforeach_current_oid() \ + (aforeach__state.l->elements[aforeach__state.i].oid_value) +#define aforeach_current_node(type) \ + castNode(type, aforeach_current()) + +/* + * aforeach_delete_current - + * delete the current list element from the List associated with the most + * closely nested aforeach() loop, returning the new List pointer. + * + * This is equivalent to list_delete_nth_cell(), but it also adjusts the + * aforeach loop's state so that no list elements will be missed. Do not + * delete elements from an active aforeach loop's list in any other way! + */ +#define aforeach_delete_current() \ + ((List *) (aforeach__state.l = list_delete_nth_cell(aforeach__state.l, \ + aforeach__state.i--))) + +/* + * aforeach_current_index - + * get the zero-based list index of the most closely nested aforeach() + * loop's current element. + * + * Beware of using this after aforeach_delete_current(); the value will be + * out of sync for the rest of the current loop iteration. Anyway, since + * you just deleted the current element, the value is pretty meaningless. + */ +#define aforeach_current_index() (aforeach__state.i) + +/* * for_each_cell - * a convenience macro which loops through a list starting from a * specified cell
pgsql-hackers by date: