* [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator @ 2021-05-01 16:29 Jason Merrill 2021-05-13 19:26 ` Jason Merrill 0 siblings, 1 reply; 8+ messages in thread From: Jason Merrill @ 2021-05-01 16:29 UTC (permalink / raw) To: gcc-patches Like my recent patch to add ovl_range and lkp_range in the C++ front end, this patch adds the tsi_range adaptor for using C++11 range-based 'for' with a STATEMENT_LIST, e.g. for (tree stmt : tsi_range (stmt_list)) { ... } This also involves adding some operators to tree_stmt_iterator that are needed for range-for iterators, and should also be useful in code that uses the iterators directly. The patch updates the suitable loops in the C++ front end, but does not touch any loops elsewhere in the compiler. gcc/ChangeLog: * tree-iterator.h (struct tree_stmt_iterator): Add operator++, operator--, operator*, operator==, and operator!=. (class tsi_range): New. gcc/cp/ChangeLog: * constexpr.c (build_data_member_initialization): Use tsi_range. (build_constexpr_constructor_member_initializers): Likewise. (constexpr_fn_retval, cxx_eval_statement_list): Likewise. (potential_constant_expression_1): Likewise. * coroutines.cc (await_statement_expander): Likewise. (await_statement_walker): Likewise. * module.cc (trees_out::core_vals): Likewise. * pt.c (tsubst_expr): Likewise. * semantics.c (set_cleanup_locs): Likewise. --- gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- gcc/cp/coroutines.cc | 10 ++++------ gcc/cp/module.cc | 5 ++--- gcc/cp/pt.c | 5 ++--- gcc/cp/semantics.c | 5 ++--- 6 files changed, 47 insertions(+), 48 deletions(-) diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h index 076fff8644c..f57456bb473 100644 --- a/gcc/tree-iterator.h +++ b/gcc/tree-iterator.h @@ -1,4 +1,4 @@ -/* Iterator routines for manipulating GENERIC tree statement list. +/* Iterator routines for manipulating GENERIC tree statement list. -*- C++ -*- Copyright (C) 2003-2021 Free Software Foundation, Inc. Contributed by Andrew MacLeod <amacleod@redhat.com> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see struct tree_stmt_iterator { struct tree_statement_list_node *ptr; tree container; + + bool operator== (tree_stmt_iterator b) const + { return b.ptr == ptr && b.container == container; } + bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } + tree &operator* () { return ptr->stmt; } }; static inline tree_stmt_iterator @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) static inline void tsi_next (tree_stmt_iterator *i) { - i->ptr = i->ptr->next; + ++(*i); } static inline void tsi_prev (tree_stmt_iterator *i) { - i->ptr = i->ptr->prev; + --(*i); } static inline tree * tsi_stmt_ptr (tree_stmt_iterator i) { - return &i.ptr->stmt; + return &(*i); } static inline tree tsi_stmt (tree_stmt_iterator i) { - return i.ptr->stmt; + return *i; } +/* Make tree_stmt_iterator work as a C++ range, e.g. + for (tree stmt : tsi_range (stmt_list)) { ... } */ +class tsi_range +{ + tree t; + public: + tsi_range (tree t): t(t) { } + tree_stmt_iterator begin() { return tsi_start (t); } + tree_stmt_iterator end() { return { nullptr, t }; } +}; + enum tsi_iterator_update { TSI_NEW_STMT, /* Only valid when single statement is added, move diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 9481a5bfd3c..260b0122f59 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, vec<constructor_elt, va_gc> **vec) return false; if (TREE_CODE (t) == STATEMENT_LIST) { - tree_stmt_iterator i; - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) - { - if (! build_data_member_initialization (tsi_stmt (i), vec)) - return false; - } + for (tree stmt : tsi_range (t)) + if (! build_data_member_initialization (stmt, vec)) + return false; return true; } if (TREE_CODE (t) == CLEANUP_STMT) @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers (tree type, tree body) break; case STATEMENT_LIST: - for (tree_stmt_iterator i = tsi_start (body); - !tsi_end_p (i); tsi_next (&i)) + for (tree stmt : tsi_range (body)) { - body = tsi_stmt (i); + body = stmt; if (TREE_CODE (body) == BIND_EXPR) break; } @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers (tree type, tree body) } else if (TREE_CODE (body) == STATEMENT_LIST) { - tree_stmt_iterator i; - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) + for (tree stmt : tsi_range (body)) { - ok = build_data_member_initialization (tsi_stmt (i), &vec); + ok = build_data_member_initialization (stmt, &vec); if (!ok) break; } @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) { case STATEMENT_LIST: { - tree_stmt_iterator i; tree expr = NULL_TREE; - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) + for (tree stmt : tsi_range (body)) { - tree s = constexpr_fn_retval (tsi_stmt (i)); + tree s = constexpr_fn_retval (stmt); if (s == error_mark_node) return error_mark_node; else if (s == NULL_TREE) @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx *ctx, tree t, bool *non_constant_p, bool *overflow_p, tree *jump_target) { - tree_stmt_iterator i; tree local_target; /* In a statement-expression we want to return the last value. For empty statement expression return void_node. */ @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx *ctx, tree t, local_target = NULL_TREE; jump_target = &local_target; } - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) + for (tree stmt : tsi_range (t)) { - tree stmt = tsi_stmt (i); /* We've found a continue, so skip everything until we reach the label its jumping to. */ if (continues (jump_target)) @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now, } case STATEMENT_LIST: - { - tree_stmt_iterator i; - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) - { - if (!RECUR (tsi_stmt (i), any)) - return false; - } - return true; - } - break; + for (tree stmt : tsi_range (t)) + if (!RECUR (stmt, any)) + return false; + return true; case MODIFY_EXPR: if (cxx_dialect < cxx14) diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc index dbd703a67cc..9b498f9d0b4 100644 --- a/gcc/cp/coroutines.cc +++ b/gcc/cp/coroutines.cc @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int *do_subtree, void *d) return NULL_TREE; /* Just process the sub-trees. */ else if (TREE_CODE (*stmt) == STATEMENT_LIST) { - tree_stmt_iterator i; - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) + for (tree &s : tsi_range (*stmt)) { - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, + res = cp_walk_tree (&s, await_statement_expander, d, NULL); if (res) return res; @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) } else if (TREE_CODE (*stmt) == STATEMENT_LIST) { - tree_stmt_iterator i; - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) + for (tree &s : tsi_range (*stmt)) { - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, + res = cp_walk_tree (&s, await_statement_walker, d, NULL); if (res) return res; diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc index 02c19f55548..f0fb0144706 100644 --- a/gcc/cp/module.cc +++ b/gcc/cp/module.cc @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) break; case STATEMENT_LIST: - for (tree_stmt_iterator iter = tsi_start (t); - !tsi_end_p (iter); tsi_next (&iter)) - if (tree stmt = tsi_stmt (iter)) + for (tree stmt : tsi_range (t)) + if (stmt) WT (stmt); WT (NULL_TREE); break; diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c index 116bdd2e42a..ad140cfd586 100644 --- a/gcc/cp/pt.c +++ b/gcc/cp/pt.c @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, { case STATEMENT_LIST: { - tree_stmt_iterator i; - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) - RECUR (tsi_stmt (i)); + for (tree stmt : tsi_range (t)) + RECUR (stmt); break; } diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c index 6224f49f189..2912efad9be 100644 --- a/gcc/cp/semantics.c +++ b/gcc/cp/semantics.c @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) set_cleanup_locs (CLEANUP_BODY (stmts), loc); } else if (TREE_CODE (stmts) == STATEMENT_LIST) - for (tree_stmt_iterator i = tsi_start (stmts); - !tsi_end_p (i); tsi_next (&i)) - set_cleanup_locs (tsi_stmt (i), loc); + for (tree stmt : tsi_range (stmts)) + set_cleanup_locs (stmt, loc); } /* Finish a scope. */ base-commit: 3c65858787dc52b65b26fa7018587c01510f442c prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 -- 2.27.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-01 16:29 [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator Jason Merrill @ 2021-05-13 19:26 ` Jason Merrill 2021-05-13 23:21 ` Martin Sebor 0 siblings, 1 reply; 8+ messages in thread From: Jason Merrill @ 2021-05-13 19:26 UTC (permalink / raw) To: gcc-patches Ping. On 5/1/21 12:29 PM, Jason Merrill wrote: > Like my recent patch to add ovl_range and lkp_range in the C++ front end, > this patch adds the tsi_range adaptor for using C++11 range-based 'for' with > a STATEMENT_LIST, e.g. > > for (tree stmt : tsi_range (stmt_list)) { ... } > > This also involves adding some operators to tree_stmt_iterator that are > needed for range-for iterators, and should also be useful in code that uses > the iterators directly. > > The patch updates the suitable loops in the C++ front end, but does not > touch any loops elsewhere in the compiler. > > gcc/ChangeLog: > > * tree-iterator.h (struct tree_stmt_iterator): Add operator++, > operator--, operator*, operator==, and operator!=. > (class tsi_range): New. > > gcc/cp/ChangeLog: > > * constexpr.c (build_data_member_initialization): Use tsi_range. > (build_constexpr_constructor_member_initializers): Likewise. > (constexpr_fn_retval, cxx_eval_statement_list): Likewise. > (potential_constant_expression_1): Likewise. > * coroutines.cc (await_statement_expander): Likewise. > (await_statement_walker): Likewise. > * module.cc (trees_out::core_vals): Likewise. > * pt.c (tsubst_expr): Likewise. > * semantics.c (set_cleanup_locs): Likewise. > --- > gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- > gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- > gcc/cp/coroutines.cc | 10 ++++------ > gcc/cp/module.cc | 5 ++--- > gcc/cp/pt.c | 5 ++--- > gcc/cp/semantics.c | 5 ++--- > 6 files changed, 47 insertions(+), 48 deletions(-) > > diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h > index 076fff8644c..f57456bb473 100644 > --- a/gcc/tree-iterator.h > +++ b/gcc/tree-iterator.h > @@ -1,4 +1,4 @@ > -/* Iterator routines for manipulating GENERIC tree statement list. > +/* Iterator routines for manipulating GENERIC tree statement list. -*- C++ -*- > Copyright (C) 2003-2021 Free Software Foundation, Inc. > Contributed by Andrew MacLeod <amacleod@redhat.com> > > @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see > struct tree_stmt_iterator { > struct tree_statement_list_node *ptr; > tree container; > + > + bool operator== (tree_stmt_iterator b) const > + { return b.ptr == ptr && b.container == container; } > + bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } > + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } > + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } > + tree &operator* () { return ptr->stmt; } > }; > > static inline tree_stmt_iterator > @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) > static inline void > tsi_next (tree_stmt_iterator *i) > { > - i->ptr = i->ptr->next; > + ++(*i); > } > > static inline void > tsi_prev (tree_stmt_iterator *i) > { > - i->ptr = i->ptr->prev; > + --(*i); > } > > static inline tree * > tsi_stmt_ptr (tree_stmt_iterator i) > { > - return &i.ptr->stmt; > + return &(*i); > } > > static inline tree > tsi_stmt (tree_stmt_iterator i) > { > - return i.ptr->stmt; > + return *i; > } > > +/* Make tree_stmt_iterator work as a C++ range, e.g. > + for (tree stmt : tsi_range (stmt_list)) { ... } */ > +class tsi_range > +{ > + tree t; > + public: > + tsi_range (tree t): t(t) { } > + tree_stmt_iterator begin() { return tsi_start (t); } > + tree_stmt_iterator end() { return { nullptr, t }; } > +}; > + > enum tsi_iterator_update > { > TSI_NEW_STMT, /* Only valid when single statement is added, move > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 9481a5bfd3c..260b0122f59 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, vec<constructor_elt, va_gc> **vec) > return false; > if (TREE_CODE (t) == STATEMENT_LIST) > { > - tree_stmt_iterator i; > - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > - { > - if (! build_data_member_initialization (tsi_stmt (i), vec)) > - return false; > - } > + for (tree stmt : tsi_range (t)) > + if (! build_data_member_initialization (stmt, vec)) > + return false; > return true; > } > if (TREE_CODE (t) == CLEANUP_STMT) > @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers (tree type, tree body) > break; > > case STATEMENT_LIST: > - for (tree_stmt_iterator i = tsi_start (body); > - !tsi_end_p (i); tsi_next (&i)) > + for (tree stmt : tsi_range (body)) > { > - body = tsi_stmt (i); > + body = stmt; > if (TREE_CODE (body) == BIND_EXPR) > break; > } > @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers (tree type, tree body) > } > else if (TREE_CODE (body) == STATEMENT_LIST) > { > - tree_stmt_iterator i; > - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > + for (tree stmt : tsi_range (body)) > { > - ok = build_data_member_initialization (tsi_stmt (i), &vec); > + ok = build_data_member_initialization (stmt, &vec); > if (!ok) > break; > } > @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) > { > case STATEMENT_LIST: > { > - tree_stmt_iterator i; > tree expr = NULL_TREE; > - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > + for (tree stmt : tsi_range (body)) > { > - tree s = constexpr_fn_retval (tsi_stmt (i)); > + tree s = constexpr_fn_retval (stmt); > if (s == error_mark_node) > return error_mark_node; > else if (s == NULL_TREE) > @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx *ctx, tree t, > bool *non_constant_p, bool *overflow_p, > tree *jump_target) > { > - tree_stmt_iterator i; > tree local_target; > /* In a statement-expression we want to return the last value. > For empty statement expression return void_node. */ > @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx *ctx, tree t, > local_target = NULL_TREE; > jump_target = &local_target; > } > - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > + for (tree stmt : tsi_range (t)) > { > - tree stmt = tsi_stmt (i); > /* We've found a continue, so skip everything until we reach > the label its jumping to. */ > if (continues (jump_target)) > @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now, > } > > case STATEMENT_LIST: > - { > - tree_stmt_iterator i; > - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > - { > - if (!RECUR (tsi_stmt (i), any)) > - return false; > - } > - return true; > - } > - break; > + for (tree stmt : tsi_range (t)) > + if (!RECUR (stmt, any)) > + return false; > + return true; > > case MODIFY_EXPR: > if (cxx_dialect < cxx14) > diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > index dbd703a67cc..9b498f9d0b4 100644 > --- a/gcc/cp/coroutines.cc > +++ b/gcc/cp/coroutines.cc > @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int *do_subtree, void *d) > return NULL_TREE; /* Just process the sub-trees. */ > else if (TREE_CODE (*stmt) == STATEMENT_LIST) > { > - tree_stmt_iterator i; > - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > + for (tree &s : tsi_range (*stmt)) > { > - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, > + res = cp_walk_tree (&s, await_statement_expander, > d, NULL); > if (res) > return res; > @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int *do_subtree, void *d) > } > else if (TREE_CODE (*stmt) == STATEMENT_LIST) > { > - tree_stmt_iterator i; > - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > + for (tree &s : tsi_range (*stmt)) > { > - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, > + res = cp_walk_tree (&s, await_statement_walker, > d, NULL); > if (res) > return res; > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > index 02c19f55548..f0fb0144706 100644 > --- a/gcc/cp/module.cc > +++ b/gcc/cp/module.cc > @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) > break; > > case STATEMENT_LIST: > - for (tree_stmt_iterator iter = tsi_start (t); > - !tsi_end_p (iter); tsi_next (&iter)) > - if (tree stmt = tsi_stmt (iter)) > + for (tree stmt : tsi_range (t)) > + if (stmt) > WT (stmt); > WT (NULL_TREE); > break; > diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > index 116bdd2e42a..ad140cfd586 100644 > --- a/gcc/cp/pt.c > +++ b/gcc/cp/pt.c > @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl, > { > case STATEMENT_LIST: > { > - tree_stmt_iterator i; > - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > - RECUR (tsi_stmt (i)); > + for (tree stmt : tsi_range (t)) > + RECUR (stmt); > break; > } > > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > index 6224f49f189..2912efad9be 100644 > --- a/gcc/cp/semantics.c > +++ b/gcc/cp/semantics.c > @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) > set_cleanup_locs (CLEANUP_BODY (stmts), loc); > } > else if (TREE_CODE (stmts) == STATEMENT_LIST) > - for (tree_stmt_iterator i = tsi_start (stmts); > - !tsi_end_p (i); tsi_next (&i)) > - set_cleanup_locs (tsi_stmt (i), loc); > + for (tree stmt : tsi_range (stmts)) > + set_cleanup_locs (stmt, loc); > } > > /* Finish a scope. */ > > base-commit: 3c65858787dc52b65b26fa7018587c01510f442c > prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-13 19:26 ` Jason Merrill @ 2021-05-13 23:21 ` Martin Sebor 2021-05-14 1:50 ` Jason Merrill 2021-05-17 7:56 ` Richard Biener 0 siblings, 2 replies; 8+ messages in thread From: Martin Sebor @ 2021-05-13 23:21 UTC (permalink / raw) To: Jason Merrill, gcc-patches On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: > Ping. > > On 5/1/21 12:29 PM, Jason Merrill wrote: >> Like my recent patch to add ovl_range and lkp_range in the C++ front end, >> this patch adds the tsi_range adaptor for using C++11 range-based >> 'for' with >> a STATEMENT_LIST, e.g. >> >> for (tree stmt : tsi_range (stmt_list)) { ... } >> >> This also involves adding some operators to tree_stmt_iterator that are >> needed for range-for iterators, and should also be useful in code that >> uses >> the iterators directly. >> >> The patch updates the suitable loops in the C++ front end, but does not >> touch any loops elsewhere in the compiler. I like the modernization of the loops. I can't find anything terribly wrong with the iterator but let me at least pick on some nits ;) >> >> gcc/ChangeLog: >> >> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, >> operator--, operator*, operator==, and operator!=. >> (class tsi_range): New. >> >> gcc/cp/ChangeLog: >> >> * constexpr.c (build_data_member_initialization): Use tsi_range. >> (build_constexpr_constructor_member_initializers): Likewise. >> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. >> (potential_constant_expression_1): Likewise. >> * coroutines.cc (await_statement_expander): Likewise. >> (await_statement_walker): Likewise. >> * module.cc (trees_out::core_vals): Likewise. >> * pt.c (tsubst_expr): Likewise. >> * semantics.c (set_cleanup_locs): Likewise. >> --- >> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- >> gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- >> gcc/cp/coroutines.cc | 10 ++++------ >> gcc/cp/module.cc | 5 ++--- >> gcc/cp/pt.c | 5 ++--- >> gcc/cp/semantics.c | 5 ++--- >> 6 files changed, 47 insertions(+), 48 deletions(-) >> >> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h >> index 076fff8644c..f57456bb473 100644 >> --- a/gcc/tree-iterator.h >> +++ b/gcc/tree-iterator.h >> @@ -1,4 +1,4 @@ >> -/* Iterator routines for manipulating GENERIC tree statement list. >> +/* Iterator routines for manipulating GENERIC tree statement list. >> -*- C++ -*- >> Copyright (C) 2003-2021 Free Software Foundation, Inc. >> Contributed by Andrew MacLeod <amacleod@redhat.com> >> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see >> struct tree_stmt_iterator { >> struct tree_statement_list_node *ptr; >> tree container; I assume the absence of ctors is intentional. If so, I suggest to add a comment explaing why. Otherwise, I would provide one (or as many as needed). >> + >> + bool operator== (tree_stmt_iterator b) const >> + { return b.ptr == ptr && b.container == container; } >> + bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } >> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } >> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } I would suggest to add postincrement and postdecrement. >> + tree &operator* () { return ptr->stmt; } Given the pervasive lack of const-safety in GCC and the by-value semantics of the iterator this probably isn't worth it but maybe add a const overload. operator-> would probably never be used. >> }; >> static inline tree_stmt_iterator >> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) >> static inline void >> tsi_next (tree_stmt_iterator *i) >> { >> - i->ptr = i->ptr->next; >> + ++(*i); >> } >> static inline void >> tsi_prev (tree_stmt_iterator *i) >> { >> - i->ptr = i->ptr->prev; >> + --(*i); >> } >> static inline tree * >> tsi_stmt_ptr (tree_stmt_iterator i) >> { >> - return &i.ptr->stmt; >> + return &(*i); >> } >> static inline tree >> tsi_stmt (tree_stmt_iterator i) >> { >> - return i.ptr->stmt; >> + return *i; >> } >> +/* Make tree_stmt_iterator work as a C++ range, e.g. >> + for (tree stmt : tsi_range (stmt_list)) { ... } */ >> +class tsi_range >> +{ >> + tree t; >> + public: >> + tsi_range (tree t): t(t) { } >> + tree_stmt_iterator begin() { return tsi_start (t); } >> + tree_stmt_iterator end() { return { nullptr, t }; } Those member functions could be made const. Martin >> +}; >> + >> enum tsi_iterator_update >> { >> TSI_NEW_STMT, /* Only valid when single statement is added, >> move >> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >> index 9481a5bfd3c..260b0122f59 100644 >> --- a/gcc/cp/constexpr.c >> +++ b/gcc/cp/constexpr.c >> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, >> vec<constructor_elt, va_gc> **vec) >> return false; >> if (TREE_CODE (t) == STATEMENT_LIST) >> { >> - tree_stmt_iterator i; >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >> - { >> - if (! build_data_member_initialization (tsi_stmt (i), vec)) >> - return false; >> - } >> + for (tree stmt : tsi_range (t)) >> + if (! build_data_member_initialization (stmt, vec)) >> + return false; >> return true; >> } >> if (TREE_CODE (t) == CLEANUP_STMT) >> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers >> (tree type, tree body) >> break; >> case STATEMENT_LIST: >> - for (tree_stmt_iterator i = tsi_start (body); >> - !tsi_end_p (i); tsi_next (&i)) >> + for (tree stmt : tsi_range (body)) >> { >> - body = tsi_stmt (i); >> + body = stmt; >> if (TREE_CODE (body) == BIND_EXPR) >> break; >> } >> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers >> (tree type, tree body) >> } >> else if (TREE_CODE (body) == STATEMENT_LIST) >> { >> - tree_stmt_iterator i; >> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >> + for (tree stmt : tsi_range (body)) >> { >> - ok = build_data_member_initialization (tsi_stmt (i), &vec); >> + ok = build_data_member_initialization (stmt, &vec); >> if (!ok) >> break; >> } >> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) >> { >> case STATEMENT_LIST: >> { >> - tree_stmt_iterator i; >> tree expr = NULL_TREE; >> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >> + for (tree stmt : tsi_range (body)) >> { >> - tree s = constexpr_fn_retval (tsi_stmt (i)); >> + tree s = constexpr_fn_retval (stmt); >> if (s == error_mark_node) >> return error_mark_node; >> else if (s == NULL_TREE) >> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx >> *ctx, tree t, >> bool *non_constant_p, bool *overflow_p, >> tree *jump_target) >> { >> - tree_stmt_iterator i; >> tree local_target; >> /* In a statement-expression we want to return the last value. >> For empty statement expression return void_node. */ >> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx >> *ctx, tree t, >> local_target = NULL_TREE; >> jump_target = &local_target; >> } >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >> + for (tree stmt : tsi_range (t)) >> { >> - tree stmt = tsi_stmt (i); >> /* We've found a continue, so skip everything until we reach >> the label its jumping to. */ >> if (continues (jump_target)) >> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool >> want_rval, bool strict, bool now, >> } >> case STATEMENT_LIST: >> - { >> - tree_stmt_iterator i; >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >> - { >> - if (!RECUR (tsi_stmt (i), any)) >> - return false; >> - } >> - return true; >> - } >> - break; >> + for (tree stmt : tsi_range (t)) >> + if (!RECUR (stmt, any)) >> + return false; >> + return true; >> case MODIFY_EXPR: >> if (cxx_dialect < cxx14) >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >> index dbd703a67cc..9b498f9d0b4 100644 >> --- a/gcc/cp/coroutines.cc >> +++ b/gcc/cp/coroutines.cc >> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int >> *do_subtree, void *d) >> return NULL_TREE; /* Just process the sub-trees. */ >> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >> { >> - tree_stmt_iterator i; >> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >> + for (tree &s : tsi_range (*stmt)) >> { >> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, >> + res = cp_walk_tree (&s, await_statement_expander, >> d, NULL); >> if (res) >> return res; >> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int >> *do_subtree, void *d) >> } >> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >> { >> - tree_stmt_iterator i; >> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >> + for (tree &s : tsi_range (*stmt)) >> { >> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, >> + res = cp_walk_tree (&s, await_statement_walker, >> d, NULL); >> if (res) >> return res; >> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >> index 02c19f55548..f0fb0144706 100644 >> --- a/gcc/cp/module.cc >> +++ b/gcc/cp/module.cc >> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) >> break; >> case STATEMENT_LIST: >> - for (tree_stmt_iterator iter = tsi_start (t); >> - !tsi_end_p (iter); tsi_next (&iter)) >> - if (tree stmt = tsi_stmt (iter)) >> + for (tree stmt : tsi_range (t)) >> + if (stmt) >> WT (stmt); >> WT (NULL_TREE); >> break; >> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >> index 116bdd2e42a..ad140cfd586 100644 >> --- a/gcc/cp/pt.c >> +++ b/gcc/cp/pt.c >> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t >> complain, tree in_decl, >> { >> case STATEMENT_LIST: >> { >> - tree_stmt_iterator i; >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >> - RECUR (tsi_stmt (i)); >> + for (tree stmt : tsi_range (t)) >> + RECUR (stmt); >> break; >> } >> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c >> index 6224f49f189..2912efad9be 100644 >> --- a/gcc/cp/semantics.c >> +++ b/gcc/cp/semantics.c >> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) >> set_cleanup_locs (CLEANUP_BODY (stmts), loc); >> } >> else if (TREE_CODE (stmts) == STATEMENT_LIST) >> - for (tree_stmt_iterator i = tsi_start (stmts); >> - !tsi_end_p (i); tsi_next (&i)) >> - set_cleanup_locs (tsi_stmt (i), loc); >> + for (tree stmt : tsi_range (stmts)) >> + set_cleanup_locs (stmt, loc); >> } >> /* Finish a scope. */ >> >> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c >> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-13 23:21 ` Martin Sebor @ 2021-05-14 1:50 ` Jason Merrill 2021-05-17 7:56 ` Richard Biener 1 sibling, 0 replies; 8+ messages in thread From: Jason Merrill @ 2021-05-14 1:50 UTC (permalink / raw) To: Martin Sebor, gcc-patches [-- Attachment #1: Type: text/plain, Size: 12418 bytes --] On 5/13/21 7:21 PM, Martin Sebor wrote: > On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: >> Ping. >> >> On 5/1/21 12:29 PM, Jason Merrill wrote: >>> Like my recent patch to add ovl_range and lkp_range in the C++ front >>> end, >>> this patch adds the tsi_range adaptor for using C++11 range-based >>> 'for' with >>> a STATEMENT_LIST, e.g. >>> >>> for (tree stmt : tsi_range (stmt_list)) { ... } >>> >>> This also involves adding some operators to tree_stmt_iterator that are >>> needed for range-for iterators, and should also be useful in code >>> that uses >>> the iterators directly. >>> >>> The patch updates the suitable loops in the C++ front end, but does not >>> touch any loops elsewhere in the compiler. > > I like the modernization of the loops. > > I can't find anything terribly wrong with the iterator but let me > at least pick on some nits ;) > >>> >>> gcc/ChangeLog: >>> >>> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, >>> operator--, operator*, operator==, and operator!=. >>> (class tsi_range): New. >>> >>> gcc/cp/ChangeLog: >>> >>> * constexpr.c (build_data_member_initialization): Use tsi_range. >>> (build_constexpr_constructor_member_initializers): Likewise. >>> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. >>> (potential_constant_expression_1): Likewise. >>> * coroutines.cc (await_statement_expander): Likewise. >>> (await_statement_walker): Likewise. >>> * module.cc (trees_out::core_vals): Likewise. >>> * pt.c (tsubst_expr): Likewise. >>> * semantics.c (set_cleanup_locs): Likewise. >>> --- >>> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- >>> gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- >>> gcc/cp/coroutines.cc | 10 ++++------ >>> gcc/cp/module.cc | 5 ++--- >>> gcc/cp/pt.c | 5 ++--- >>> gcc/cp/semantics.c | 5 ++--- >>> 6 files changed, 47 insertions(+), 48 deletions(-) >>> >>> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h >>> index 076fff8644c..f57456bb473 100644 >>> --- a/gcc/tree-iterator.h >>> +++ b/gcc/tree-iterator.h >>> @@ -1,4 +1,4 @@ >>> -/* Iterator routines for manipulating GENERIC tree statement list. >>> +/* Iterator routines for manipulating GENERIC tree statement list. >>> -*- C++ -*- >>> Copyright (C) 2003-2021 Free Software Foundation, Inc. >>> Contributed by Andrew MacLeod <amacleod@redhat.com> >>> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see >>> struct tree_stmt_iterator { >>> struct tree_statement_list_node *ptr; >>> tree container; > > I assume the absence of ctors is intentional. If so, I suggest > to add a comment explaing why. Otherwise, I would provide one > (or as many as needed). > >>> + >>> + bool operator== (tree_stmt_iterator b) const >>> + { return b.ptr == ptr && b.container == container; } >>> + bool operator!= (tree_stmt_iterator b) const { return !(*this == >>> b); } >>> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } >>> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } > > I would suggest to add postincrement and postdecrement. > >>> + tree &operator* () { return ptr->stmt; } > > Given the pervasive lack of const-safety in GCC and the by-value > semantics of the iterator this probably isn't worth it but maybe > add a const overload. operator-> would probably never be used. > >>> }; >>> static inline tree_stmt_iterator >>> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) >>> static inline void >>> tsi_next (tree_stmt_iterator *i) >>> { >>> - i->ptr = i->ptr->next; >>> + ++(*i); >>> } >>> static inline void >>> tsi_prev (tree_stmt_iterator *i) >>> { >>> - i->ptr = i->ptr->prev; >>> + --(*i); >>> } >>> static inline tree * >>> tsi_stmt_ptr (tree_stmt_iterator i) >>> { >>> - return &i.ptr->stmt; >>> + return &(*i); >>> } >>> static inline tree >>> tsi_stmt (tree_stmt_iterator i) >>> { >>> - return i.ptr->stmt; >>> + return *i; >>> } >>> +/* Make tree_stmt_iterator work as a C++ range, e.g. >>> + for (tree stmt : tsi_range (stmt_list)) { ... } */ >>> +class tsi_range >>> +{ >>> + tree t; >>> + public: >>> + tsi_range (tree t): t(t) { } >>> + tree_stmt_iterator begin() { return tsi_start (t); } >>> + tree_stmt_iterator end() { return { nullptr, t }; } > > Those member functions could be made const. Sure: >>> +}; >>> + >>> enum tsi_iterator_update >>> { >>> TSI_NEW_STMT, /* Only valid when single statement is >>> added, move >>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>> index 9481a5bfd3c..260b0122f59 100644 >>> --- a/gcc/cp/constexpr.c >>> +++ b/gcc/cp/constexpr.c >>> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, >>> vec<constructor_elt, va_gc> **vec) >>> return false; >>> if (TREE_CODE (t) == STATEMENT_LIST) >>> { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>> - { >>> - if (! build_data_member_initialization (tsi_stmt (i), vec)) >>> - return false; >>> - } >>> + for (tree stmt : tsi_range (t)) >>> + if (! build_data_member_initialization (stmt, vec)) >>> + return false; >>> return true; >>> } >>> if (TREE_CODE (t) == CLEANUP_STMT) >>> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers >>> (tree type, tree body) >>> break; >>> case STATEMENT_LIST: >>> - for (tree_stmt_iterator i = tsi_start (body); >>> - !tsi_end_p (i); tsi_next (&i)) >>> + for (tree stmt : tsi_range (body)) >>> { >>> - body = tsi_stmt (i); >>> + body = stmt; >>> if (TREE_CODE (body) == BIND_EXPR) >>> break; >>> } >>> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers >>> (tree type, tree body) >>> } >>> else if (TREE_CODE (body) == STATEMENT_LIST) >>> { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>> + for (tree stmt : tsi_range (body)) >>> { >>> - ok = build_data_member_initialization (tsi_stmt (i), &vec); >>> + ok = build_data_member_initialization (stmt, &vec); >>> if (!ok) >>> break; >>> } >>> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) >>> { >>> case STATEMENT_LIST: >>> { >>> - tree_stmt_iterator i; >>> tree expr = NULL_TREE; >>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>> + for (tree stmt : tsi_range (body)) >>> { >>> - tree s = constexpr_fn_retval (tsi_stmt (i)); >>> + tree s = constexpr_fn_retval (stmt); >>> if (s == error_mark_node) >>> return error_mark_node; >>> else if (s == NULL_TREE) >>> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx >>> *ctx, tree t, >>> bool *non_constant_p, bool *overflow_p, >>> tree *jump_target) >>> { >>> - tree_stmt_iterator i; >>> tree local_target; >>> /* In a statement-expression we want to return the last value. >>> For empty statement expression return void_node. */ >>> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx >>> *ctx, tree t, >>> local_target = NULL_TREE; >>> jump_target = &local_target; >>> } >>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>> + for (tree stmt : tsi_range (t)) >>> { >>> - tree stmt = tsi_stmt (i); >>> /* We've found a continue, so skip everything until we reach >>> the label its jumping to. */ >>> if (continues (jump_target)) >>> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool >>> want_rval, bool strict, bool now, >>> } >>> case STATEMENT_LIST: >>> - { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>> - { >>> - if (!RECUR (tsi_stmt (i), any)) >>> - return false; >>> - } >>> - return true; >>> - } >>> - break; >>> + for (tree stmt : tsi_range (t)) >>> + if (!RECUR (stmt, any)) >>> + return false; >>> + return true; >>> case MODIFY_EXPR: >>> if (cxx_dialect < cxx14) >>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>> index dbd703a67cc..9b498f9d0b4 100644 >>> --- a/gcc/cp/coroutines.cc >>> +++ b/gcc/cp/coroutines.cc >>> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int >>> *do_subtree, void *d) >>> return NULL_TREE; /* Just process the sub-trees. */ >>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>> { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>> + for (tree &s : tsi_range (*stmt)) >>> { >>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, >>> + res = cp_walk_tree (&s, await_statement_expander, >>> d, NULL); >>> if (res) >>> return res; >>> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int >>> *do_subtree, void *d) >>> } >>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>> { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>> + for (tree &s : tsi_range (*stmt)) >>> { >>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, >>> + res = cp_walk_tree (&s, await_statement_walker, >>> d, NULL); >>> if (res) >>> return res; >>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>> index 02c19f55548..f0fb0144706 100644 >>> --- a/gcc/cp/module.cc >>> +++ b/gcc/cp/module.cc >>> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) >>> break; >>> case STATEMENT_LIST: >>> - for (tree_stmt_iterator iter = tsi_start (t); >>> - !tsi_end_p (iter); tsi_next (&iter)) >>> - if (tree stmt = tsi_stmt (iter)) >>> + for (tree stmt : tsi_range (t)) >>> + if (stmt) >>> WT (stmt); >>> WT (NULL_TREE); >>> break; >>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>> index 116bdd2e42a..ad140cfd586 100644 >>> --- a/gcc/cp/pt.c >>> +++ b/gcc/cp/pt.c >>> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, >>> tsubst_flags_t complain, tree in_decl, >>> { >>> case STATEMENT_LIST: >>> { >>> - tree_stmt_iterator i; >>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>> - RECUR (tsi_stmt (i)); >>> + for (tree stmt : tsi_range (t)) >>> + RECUR (stmt); >>> break; >>> } >>> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c >>> index 6224f49f189..2912efad9be 100644 >>> --- a/gcc/cp/semantics.c >>> +++ b/gcc/cp/semantics.c >>> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) >>> set_cleanup_locs (CLEANUP_BODY (stmts), loc); >>> } >>> else if (TREE_CODE (stmts) == STATEMENT_LIST) >>> - for (tree_stmt_iterator i = tsi_start (stmts); >>> - !tsi_end_p (i); tsi_next (&i)) >>> - set_cleanup_locs (tsi_stmt (i), loc); >>> + for (tree stmt : tsi_range (stmts)) >>> + set_cleanup_locs (stmt, loc); >>> } >>> /* Finish a scope. */ >>> >>> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c >>> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 >>> >> > [-- Attachment #2: 0001-fixup-tree-iterator-C-11-range-for-and-tree_stmt_ite.patch --] [-- Type: text/x-patch, Size: 1744 bytes --] From 964005e6f08c3eff8b5cd8dfb4e4def5dfc28709 Mon Sep 17 00:00:00 2001 From: Jason Merrill <jason@redhat.com> Date: Thu, 13 May 2021 21:21:23 -0400 Subject: [PATCH] fixup! tree-iterator: C++11 range-for and tree_stmt_iterator To: gcc-patches@gcc.gnu.org --- gcc/tree-iterator.h | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h index f57456bb473..a72d0d37f1c 100644 --- a/gcc/tree-iterator.h +++ b/gcc/tree-iterator.h @@ -33,12 +33,20 @@ struct tree_stmt_iterator { struct tree_statement_list_node *ptr; tree container; + /* No need for user-defined constructors, the implicit definitions (or + aggregate initialization) are fine. */ + bool operator== (tree_stmt_iterator b) const { return b.ptr == ptr && b.container == container; } bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } + tree_stmt_iterator operator++ (int) + { tree_stmt_iterator x = *this; ++*this; return x; } + tree_stmt_iterator operator-- (int) + { tree_stmt_iterator x = *this; --*this; return x; } tree &operator* () { return ptr->stmt; } + tree operator* () const { return ptr->stmt; } }; static inline tree_stmt_iterator @@ -106,8 +114,8 @@ class tsi_range tree t; public: tsi_range (tree t): t(t) { } - tree_stmt_iterator begin() { return tsi_start (t); } - tree_stmt_iterator end() { return { nullptr, t }; } + tree_stmt_iterator begin() const { return tsi_start (t); } + tree_stmt_iterator end() const { return { nullptr, t }; } }; enum tsi_iterator_update -- 2.27.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-13 23:21 ` Martin Sebor 2021-05-14 1:50 ` Jason Merrill @ 2021-05-17 7:56 ` Richard Biener 2021-05-17 17:58 ` Jason Merrill 1 sibling, 1 reply; 8+ messages in thread From: Richard Biener @ 2021-05-17 7:56 UTC (permalink / raw) To: Martin Sebor; +Cc: Jason Merrill, GCC Patches On Fri, May 14, 2021 at 2:23 AM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: > > Ping. > > > > On 5/1/21 12:29 PM, Jason Merrill wrote: > >> Like my recent patch to add ovl_range and lkp_range in the C++ front end, > >> this patch adds the tsi_range adaptor for using C++11 range-based > >> 'for' with > >> a STATEMENT_LIST, e.g. > >> > >> for (tree stmt : tsi_range (stmt_list)) { ... } > >> > >> This also involves adding some operators to tree_stmt_iterator that are > >> needed for range-for iterators, and should also be useful in code that > >> uses > >> the iterators directly. > >> > >> The patch updates the suitable loops in the C++ front end, but does not > >> touch any loops elsewhere in the compiler. > > I like the modernization of the loops. The only worry I have (and why I stopped looking at range-for) is that this adds another style of looping over stmts without opening the possibility to remove another or even unify all of them. That's because range-for isn't powerful enough w/o jumping through hoops and/or we cannot use what appearantly ranges<> was intended for (fix this limitation). That said, if some C++ literate could see if for example what gimple-iterator.h provides can be completely modernized then that would be great of course. There's stuff like reverse iteration, iteration skipping debug stmts, compares of iterators like gsi_one_before_end_p, etc. Given my failed tries (but I'm a C++ illiterate) my TODO list now only contains turning the iterators into STL style ones, thus gsi_stmt (it) -> *it, gsi_next (&it) -> ++it, etc. - but even it != end_p looks a bit awkward there. Richard. > I can't find anything terribly wrong with the iterator but let me > at least pick on some nits ;) > > >> > >> gcc/ChangeLog: > >> > >> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, > >> operator--, operator*, operator==, and operator!=. > >> (class tsi_range): New. > >> > >> gcc/cp/ChangeLog: > >> > >> * constexpr.c (build_data_member_initialization): Use tsi_range. > >> (build_constexpr_constructor_member_initializers): Likewise. > >> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. > >> (potential_constant_expression_1): Likewise. > >> * coroutines.cc (await_statement_expander): Likewise. > >> (await_statement_walker): Likewise. > >> * module.cc (trees_out::core_vals): Likewise. > >> * pt.c (tsubst_expr): Likewise. > >> * semantics.c (set_cleanup_locs): Likewise. > >> --- > >> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- > >> gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- > >> gcc/cp/coroutines.cc | 10 ++++------ > >> gcc/cp/module.cc | 5 ++--- > >> gcc/cp/pt.c | 5 ++--- > >> gcc/cp/semantics.c | 5 ++--- > >> 6 files changed, 47 insertions(+), 48 deletions(-) > >> > >> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h > >> index 076fff8644c..f57456bb473 100644 > >> --- a/gcc/tree-iterator.h > >> +++ b/gcc/tree-iterator.h > >> @@ -1,4 +1,4 @@ > >> -/* Iterator routines for manipulating GENERIC tree statement list. > >> +/* Iterator routines for manipulating GENERIC tree statement list. > >> -*- C++ -*- > >> Copyright (C) 2003-2021 Free Software Foundation, Inc. > >> Contributed by Andrew MacLeod <amacleod@redhat.com> > >> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see > >> struct tree_stmt_iterator { > >> struct tree_statement_list_node *ptr; > >> tree container; > > I assume the absence of ctors is intentional. If so, I suggest > to add a comment explaing why. Otherwise, I would provide one > (or as many as needed). > > >> + > >> + bool operator== (tree_stmt_iterator b) const > >> + { return b.ptr == ptr && b.container == container; } > >> + bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } > >> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } > >> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } > > I would suggest to add postincrement and postdecrement. > > >> + tree &operator* () { return ptr->stmt; } > > Given the pervasive lack of const-safety in GCC and the by-value > semantics of the iterator this probably isn't worth it but maybe > add a const overload. operator-> would probably never be used. > > >> }; > >> static inline tree_stmt_iterator > >> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) > >> static inline void > >> tsi_next (tree_stmt_iterator *i) > >> { > >> - i->ptr = i->ptr->next; > >> + ++(*i); > >> } > >> static inline void > >> tsi_prev (tree_stmt_iterator *i) > >> { > >> - i->ptr = i->ptr->prev; > >> + --(*i); > >> } > >> static inline tree * > >> tsi_stmt_ptr (tree_stmt_iterator i) > >> { > >> - return &i.ptr->stmt; > >> + return &(*i); > >> } > >> static inline tree > >> tsi_stmt (tree_stmt_iterator i) > >> { > >> - return i.ptr->stmt; > >> + return *i; > >> } > >> +/* Make tree_stmt_iterator work as a C++ range, e.g. > >> + for (tree stmt : tsi_range (stmt_list)) { ... } */ > >> +class tsi_range > >> +{ > >> + tree t; > >> + public: > >> + tsi_range (tree t): t(t) { } > >> + tree_stmt_iterator begin() { return tsi_start (t); } > >> + tree_stmt_iterator end() { return { nullptr, t }; } > > Those member functions could be made const. > > Martin > > >> +}; > >> + > >> enum tsi_iterator_update > >> { > >> TSI_NEW_STMT, /* Only valid when single statement is added, > >> move > >> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > >> index 9481a5bfd3c..260b0122f59 100644 > >> --- a/gcc/cp/constexpr.c > >> +++ b/gcc/cp/constexpr.c > >> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, > >> vec<constructor_elt, va_gc> **vec) > >> return false; > >> if (TREE_CODE (t) == STATEMENT_LIST) > >> { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >> - { > >> - if (! build_data_member_initialization (tsi_stmt (i), vec)) > >> - return false; > >> - } > >> + for (tree stmt : tsi_range (t)) > >> + if (! build_data_member_initialization (stmt, vec)) > >> + return false; > >> return true; > >> } > >> if (TREE_CODE (t) == CLEANUP_STMT) > >> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers > >> (tree type, tree body) > >> break; > >> case STATEMENT_LIST: > >> - for (tree_stmt_iterator i = tsi_start (body); > >> - !tsi_end_p (i); tsi_next (&i)) > >> + for (tree stmt : tsi_range (body)) > >> { > >> - body = tsi_stmt (i); > >> + body = stmt; > >> if (TREE_CODE (body) == BIND_EXPR) > >> break; > >> } > >> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers > >> (tree type, tree body) > >> } > >> else if (TREE_CODE (body) == STATEMENT_LIST) > >> { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > >> + for (tree stmt : tsi_range (body)) > >> { > >> - ok = build_data_member_initialization (tsi_stmt (i), &vec); > >> + ok = build_data_member_initialization (stmt, &vec); > >> if (!ok) > >> break; > >> } > >> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) > >> { > >> case STATEMENT_LIST: > >> { > >> - tree_stmt_iterator i; > >> tree expr = NULL_TREE; > >> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > >> + for (tree stmt : tsi_range (body)) > >> { > >> - tree s = constexpr_fn_retval (tsi_stmt (i)); > >> + tree s = constexpr_fn_retval (stmt); > >> if (s == error_mark_node) > >> return error_mark_node; > >> else if (s == NULL_TREE) > >> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx > >> *ctx, tree t, > >> bool *non_constant_p, bool *overflow_p, > >> tree *jump_target) > >> { > >> - tree_stmt_iterator i; > >> tree local_target; > >> /* In a statement-expression we want to return the last value. > >> For empty statement expression return void_node. */ > >> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx > >> *ctx, tree t, > >> local_target = NULL_TREE; > >> jump_target = &local_target; > >> } > >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >> + for (tree stmt : tsi_range (t)) > >> { > >> - tree stmt = tsi_stmt (i); > >> /* We've found a continue, so skip everything until we reach > >> the label its jumping to. */ > >> if (continues (jump_target)) > >> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool > >> want_rval, bool strict, bool now, > >> } > >> case STATEMENT_LIST: > >> - { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >> - { > >> - if (!RECUR (tsi_stmt (i), any)) > >> - return false; > >> - } > >> - return true; > >> - } > >> - break; > >> + for (tree stmt : tsi_range (t)) > >> + if (!RECUR (stmt, any)) > >> + return false; > >> + return true; > >> case MODIFY_EXPR: > >> if (cxx_dialect < cxx14) > >> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > >> index dbd703a67cc..9b498f9d0b4 100644 > >> --- a/gcc/cp/coroutines.cc > >> +++ b/gcc/cp/coroutines.cc > >> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int > >> *do_subtree, void *d) > >> return NULL_TREE; /* Just process the sub-trees. */ > >> else if (TREE_CODE (*stmt) == STATEMENT_LIST) > >> { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > >> + for (tree &s : tsi_range (*stmt)) > >> { > >> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, > >> + res = cp_walk_tree (&s, await_statement_expander, > >> d, NULL); > >> if (res) > >> return res; > >> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int > >> *do_subtree, void *d) > >> } > >> else if (TREE_CODE (*stmt) == STATEMENT_LIST) > >> { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > >> + for (tree &s : tsi_range (*stmt)) > >> { > >> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, > >> + res = cp_walk_tree (&s, await_statement_walker, > >> d, NULL); > >> if (res) > >> return res; > >> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > >> index 02c19f55548..f0fb0144706 100644 > >> --- a/gcc/cp/module.cc > >> +++ b/gcc/cp/module.cc > >> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) > >> break; > >> case STATEMENT_LIST: > >> - for (tree_stmt_iterator iter = tsi_start (t); > >> - !tsi_end_p (iter); tsi_next (&iter)) > >> - if (tree stmt = tsi_stmt (iter)) > >> + for (tree stmt : tsi_range (t)) > >> + if (stmt) > >> WT (stmt); > >> WT (NULL_TREE); > >> break; > >> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > >> index 116bdd2e42a..ad140cfd586 100644 > >> --- a/gcc/cp/pt.c > >> +++ b/gcc/cp/pt.c > >> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t > >> complain, tree in_decl, > >> { > >> case STATEMENT_LIST: > >> { > >> - tree_stmt_iterator i; > >> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >> - RECUR (tsi_stmt (i)); > >> + for (tree stmt : tsi_range (t)) > >> + RECUR (stmt); > >> break; > >> } > >> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > >> index 6224f49f189..2912efad9be 100644 > >> --- a/gcc/cp/semantics.c > >> +++ b/gcc/cp/semantics.c > >> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) > >> set_cleanup_locs (CLEANUP_BODY (stmts), loc); > >> } > >> else if (TREE_CODE (stmts) == STATEMENT_LIST) > >> - for (tree_stmt_iterator i = tsi_start (stmts); > >> - !tsi_end_p (i); tsi_next (&i)) > >> - set_cleanup_locs (tsi_stmt (i), loc); > >> + for (tree stmt : tsi_range (stmts)) > >> + set_cleanup_locs (stmt, loc); > >> } > >> /* Finish a scope. */ > >> > >> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c > >> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 > >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-17 7:56 ` Richard Biener @ 2021-05-17 17:58 ` Jason Merrill 2021-05-26 13:56 ` Jason Merrill 0 siblings, 1 reply; 8+ messages in thread From: Jason Merrill @ 2021-05-17 17:58 UTC (permalink / raw) To: Richard Biener, Martin Sebor; +Cc: GCC Patches On 5/17/21 3:56 AM, Richard Biener wrote: > On Fri, May 14, 2021 at 2:23 AM Martin Sebor via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: >>> Ping. >>> >>> On 5/1/21 12:29 PM, Jason Merrill wrote: >>>> Like my recent patch to add ovl_range and lkp_range in the C++ front end, >>>> this patch adds the tsi_range adaptor for using C++11 range-based >>>> 'for' with >>>> a STATEMENT_LIST, e.g. >>>> >>>> for (tree stmt : tsi_range (stmt_list)) { ... } >>>> >>>> This also involves adding some operators to tree_stmt_iterator that are >>>> needed for range-for iterators, and should also be useful in code that >>>> uses >>>> the iterators directly. >>>> >>>> The patch updates the suitable loops in the C++ front end, but does not >>>> touch any loops elsewhere in the compiler. >> >> I like the modernization of the loops. > > The only worry I have (and why I stopped looking at range-for) is that > this adds another style of looping over stmts without opening the > possibility to remove another or even unify all of them. That's because > range-for isn't powerful enough w/o jumping through hoops and/or > we cannot use what appearantly ranges<> was intended for (fix > this limitation). The range-for enabled by my patch simplifies the common case of simple iteration over elements; that seems worth doing to me even if it doesn't replace all loops. Just as FOR_EACH_VEC_ELT isn't suitable for all loops over a vector. > That said, if some C++ literate could see if for example > what gimple-iterator.h provides can be completely modernized > then that would be great of course. > > There's stuff like reverse iteration This is typically done with the reverse_iterator<> adaptor, which we could get from <iterator> or duplicate. I didn't see enough reverse iterations to make it seem worth the bother. > iteration skipping debug stmts, There you can move the condition into the loop: if (gimple_code (stmt) == GIMPLE_DEBUG) continue; > compares of iterators like gsi_one_before_end_p, etc. Certainly anything where you want to mess with the iterators directly doesn't really translate to range-for. > Given my failed tries (but I'm a C++ illiterate) my TODO list now > only contains turning the iterators into STL style ones, thus > gsi_stmt (it) -> *it, gsi_next (&it) -> ++it, etc. - but even > it != end_p looks a bit awkward there. Well, it < end_val is pretty typical for loops involving integer iterators. But you don't have to use that style if you'd rather not. You could continue to use gsi_end_p, or just *it, since we know that *it is NULL at the end of the sequence. >> I can't find anything terribly wrong with the iterator but let me >> at least pick on some nits ;) >> >>>> >>>> gcc/ChangeLog: >>>> >>>> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, >>>> operator--, operator*, operator==, and operator!=. >>>> (class tsi_range): New. >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> * constexpr.c (build_data_member_initialization): Use tsi_range. >>>> (build_constexpr_constructor_member_initializers): Likewise. >>>> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. >>>> (potential_constant_expression_1): Likewise. >>>> * coroutines.cc (await_statement_expander): Likewise. >>>> (await_statement_walker): Likewise. >>>> * module.cc (trees_out::core_vals): Likewise. >>>> * pt.c (tsubst_expr): Likewise. >>>> * semantics.c (set_cleanup_locs): Likewise. >>>> --- >>>> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- >>>> gcc/cp/constexpr.c | 42 ++++++++++++++---------------------------- >>>> gcc/cp/coroutines.cc | 10 ++++------ >>>> gcc/cp/module.cc | 5 ++--- >>>> gcc/cp/pt.c | 5 ++--- >>>> gcc/cp/semantics.c | 5 ++--- >>>> 6 files changed, 47 insertions(+), 48 deletions(-) >>>> >>>> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h >>>> index 076fff8644c..f57456bb473 100644 >>>> --- a/gcc/tree-iterator.h >>>> +++ b/gcc/tree-iterator.h >>>> @@ -1,4 +1,4 @@ >>>> -/* Iterator routines for manipulating GENERIC tree statement list. >>>> +/* Iterator routines for manipulating GENERIC tree statement list. >>>> -*- C++ -*- >>>> Copyright (C) 2003-2021 Free Software Foundation, Inc. >>>> Contributed by Andrew MacLeod <amacleod@redhat.com> >>>> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see >>>> struct tree_stmt_iterator { >>>> struct tree_statement_list_node *ptr; >>>> tree container; >> >> I assume the absence of ctors is intentional. If so, I suggest >> to add a comment explaing why. Otherwise, I would provide one >> (or as many as needed). >> >>>> + >>>> + bool operator== (tree_stmt_iterator b) const >>>> + { return b.ptr == ptr && b.container == container; } >>>> + bool operator!= (tree_stmt_iterator b) const { return !(*this == b); } >>>> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return *this; } >>>> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return *this; } >> >> I would suggest to add postincrement and postdecrement. >> >>>> + tree &operator* () { return ptr->stmt; } >> >> Given the pervasive lack of const-safety in GCC and the by-value >> semantics of the iterator this probably isn't worth it but maybe >> add a const overload. operator-> would probably never be used. >> >>>> }; >>>> static inline tree_stmt_iterator >>>> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) >>>> static inline void >>>> tsi_next (tree_stmt_iterator *i) >>>> { >>>> - i->ptr = i->ptr->next; >>>> + ++(*i); >>>> } >>>> static inline void >>>> tsi_prev (tree_stmt_iterator *i) >>>> { >>>> - i->ptr = i->ptr->prev; >>>> + --(*i); >>>> } >>>> static inline tree * >>>> tsi_stmt_ptr (tree_stmt_iterator i) >>>> { >>>> - return &i.ptr->stmt; >>>> + return &(*i); >>>> } >>>> static inline tree >>>> tsi_stmt (tree_stmt_iterator i) >>>> { >>>> - return i.ptr->stmt; >>>> + return *i; >>>> } >>>> +/* Make tree_stmt_iterator work as a C++ range, e.g. >>>> + for (tree stmt : tsi_range (stmt_list)) { ... } */ >>>> +class tsi_range >>>> +{ >>>> + tree t; >>>> + public: >>>> + tsi_range (tree t): t(t) { } >>>> + tree_stmt_iterator begin() { return tsi_start (t); } >>>> + tree_stmt_iterator end() { return { nullptr, t }; } >> >> Those member functions could be made const. >> >> Martin >> >>>> +}; >>>> + >>>> enum tsi_iterator_update >>>> { >>>> TSI_NEW_STMT, /* Only valid when single statement is added, >>>> move >>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>> index 9481a5bfd3c..260b0122f59 100644 >>>> --- a/gcc/cp/constexpr.c >>>> +++ b/gcc/cp/constexpr.c >>>> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, >>>> vec<constructor_elt, va_gc> **vec) >>>> return false; >>>> if (TREE_CODE (t) == STATEMENT_LIST) >>>> { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>> - { >>>> - if (! build_data_member_initialization (tsi_stmt (i), vec)) >>>> - return false; >>>> - } >>>> + for (tree stmt : tsi_range (t)) >>>> + if (! build_data_member_initialization (stmt, vec)) >>>> + return false; >>>> return true; >>>> } >>>> if (TREE_CODE (t) == CLEANUP_STMT) >>>> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers >>>> (tree type, tree body) >>>> break; >>>> case STATEMENT_LIST: >>>> - for (tree_stmt_iterator i = tsi_start (body); >>>> - !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree stmt : tsi_range (body)) >>>> { >>>> - body = tsi_stmt (i); >>>> + body = stmt; >>>> if (TREE_CODE (body) == BIND_EXPR) >>>> break; >>>> } >>>> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers >>>> (tree type, tree body) >>>> } >>>> else if (TREE_CODE (body) == STATEMENT_LIST) >>>> { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree stmt : tsi_range (body)) >>>> { >>>> - ok = build_data_member_initialization (tsi_stmt (i), &vec); >>>> + ok = build_data_member_initialization (stmt, &vec); >>>> if (!ok) >>>> break; >>>> } >>>> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) >>>> { >>>> case STATEMENT_LIST: >>>> { >>>> - tree_stmt_iterator i; >>>> tree expr = NULL_TREE; >>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree stmt : tsi_range (body)) >>>> { >>>> - tree s = constexpr_fn_retval (tsi_stmt (i)); >>>> + tree s = constexpr_fn_retval (stmt); >>>> if (s == error_mark_node) >>>> return error_mark_node; >>>> else if (s == NULL_TREE) >>>> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx >>>> *ctx, tree t, >>>> bool *non_constant_p, bool *overflow_p, >>>> tree *jump_target) >>>> { >>>> - tree_stmt_iterator i; >>>> tree local_target; >>>> /* In a statement-expression we want to return the last value. >>>> For empty statement expression return void_node. */ >>>> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx >>>> *ctx, tree t, >>>> local_target = NULL_TREE; >>>> jump_target = &local_target; >>>> } >>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree stmt : tsi_range (t)) >>>> { >>>> - tree stmt = tsi_stmt (i); >>>> /* We've found a continue, so skip everything until we reach >>>> the label its jumping to. */ >>>> if (continues (jump_target)) >>>> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool >>>> want_rval, bool strict, bool now, >>>> } >>>> case STATEMENT_LIST: >>>> - { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>> - { >>>> - if (!RECUR (tsi_stmt (i), any)) >>>> - return false; >>>> - } >>>> - return true; >>>> - } >>>> - break; >>>> + for (tree stmt : tsi_range (t)) >>>> + if (!RECUR (stmt, any)) >>>> + return false; >>>> + return true; >>>> case MODIFY_EXPR: >>>> if (cxx_dialect < cxx14) >>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>>> index dbd703a67cc..9b498f9d0b4 100644 >>>> --- a/gcc/cp/coroutines.cc >>>> +++ b/gcc/cp/coroutines.cc >>>> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int >>>> *do_subtree, void *d) >>>> return NULL_TREE; /* Just process the sub-trees. */ >>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>>> { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree &s : tsi_range (*stmt)) >>>> { >>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, >>>> + res = cp_walk_tree (&s, await_statement_expander, >>>> d, NULL); >>>> if (res) >>>> return res; >>>> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int >>>> *do_subtree, void *d) >>>> } >>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>>> { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>>> + for (tree &s : tsi_range (*stmt)) >>>> { >>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, >>>> + res = cp_walk_tree (&s, await_statement_walker, >>>> d, NULL); >>>> if (res) >>>> return res; >>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>> index 02c19f55548..f0fb0144706 100644 >>>> --- a/gcc/cp/module.cc >>>> +++ b/gcc/cp/module.cc >>>> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) >>>> break; >>>> case STATEMENT_LIST: >>>> - for (tree_stmt_iterator iter = tsi_start (t); >>>> - !tsi_end_p (iter); tsi_next (&iter)) >>>> - if (tree stmt = tsi_stmt (iter)) >>>> + for (tree stmt : tsi_range (t)) >>>> + if (stmt) >>>> WT (stmt); >>>> WT (NULL_TREE); >>>> break; >>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>>> index 116bdd2e42a..ad140cfd586 100644 >>>> --- a/gcc/cp/pt.c >>>> +++ b/gcc/cp/pt.c >>>> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t >>>> complain, tree in_decl, >>>> { >>>> case STATEMENT_LIST: >>>> { >>>> - tree_stmt_iterator i; >>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>> - RECUR (tsi_stmt (i)); >>>> + for (tree stmt : tsi_range (t)) >>>> + RECUR (stmt); >>>> break; >>>> } >>>> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c >>>> index 6224f49f189..2912efad9be 100644 >>>> --- a/gcc/cp/semantics.c >>>> +++ b/gcc/cp/semantics.c >>>> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) >>>> set_cleanup_locs (CLEANUP_BODY (stmts), loc); >>>> } >>>> else if (TREE_CODE (stmts) == STATEMENT_LIST) >>>> - for (tree_stmt_iterator i = tsi_start (stmts); >>>> - !tsi_end_p (i); tsi_next (&i)) >>>> - set_cleanup_locs (tsi_stmt (i), loc); >>>> + for (tree stmt : tsi_range (stmts)) >>>> + set_cleanup_locs (stmt, loc); >>>> } >>>> /* Finish a scope. */ >>>> >>>> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c >>>> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 >>>> >>> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-17 17:58 ` Jason Merrill @ 2021-05-26 13:56 ` Jason Merrill 2021-05-28 6:53 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Jason Merrill @ 2021-05-26 13:56 UTC (permalink / raw) To: Richard Biener, Martin Sebor; +Cc: GCC Patches Ping. On 5/17/21 1:58 PM, Jason Merrill wrote: > On 5/17/21 3:56 AM, Richard Biener wrote: >> On Fri, May 14, 2021 at 2:23 AM Martin Sebor via Gcc-patches >> <gcc-patches@gcc.gnu.org> wrote: >>> >>> On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: >>>> Ping. >>>> >>>> On 5/1/21 12:29 PM, Jason Merrill wrote: >>>>> Like my recent patch to add ovl_range and lkp_range in the C++ >>>>> front end, >>>>> this patch adds the tsi_range adaptor for using C++11 range-based >>>>> 'for' with >>>>> a STATEMENT_LIST, e.g. >>>>> >>>>> for (tree stmt : tsi_range (stmt_list)) { ... } >>>>> >>>>> This also involves adding some operators to tree_stmt_iterator that >>>>> are >>>>> needed for range-for iterators, and should also be useful in code that >>>>> uses >>>>> the iterators directly. >>>>> >>>>> The patch updates the suitable loops in the C++ front end, but does >>>>> not >>>>> touch any loops elsewhere in the compiler. >>> >>> I like the modernization of the loops. >> >> The only worry I have (and why I stopped looking at range-for) is that >> this adds another style of looping over stmts without opening the >> possibility to remove another or even unify all of them. That's because >> range-for isn't powerful enough w/o jumping through hoops and/or >> we cannot use what appearantly ranges<> was intended for (fix >> this limitation). > > The range-for enabled by my patch simplifies the common case of simple > iteration over elements; that seems worth doing to me even if it doesn't > replace all loops. Just as FOR_EACH_VEC_ELT isn't suitable for all > loops over a vector. > >> That said, if some C++ literate could see if for example >> what gimple-iterator.h provides can be completely modernized >> then that would be great of course. >> >> There's stuff like reverse iteration > > This is typically done with the reverse_iterator<> adaptor, which we > could get from <iterator> or duplicate. I didn't see enough reverse > iterations to make it seem worth the bother. > >> iteration skipping debug stmts, > > There you can move the condition into the loop: > > if (gimple_code (stmt) == GIMPLE_DEBUG) > continue; > >> compares of iterators like gsi_one_before_end_p, etc. > > Certainly anything where you want to mess with the iterators directly > doesn't really translate to range-for. > >> Given my failed tries (but I'm a C++ illiterate) my TODO list now >> only contains turning the iterators into STL style ones, thus >> gsi_stmt (it) -> *it, gsi_next (&it) -> ++it, etc. - but even >> it != end_p looks a bit awkward there. > > Well, it < end_val is pretty typical for loops involving integer > iterators. But you don't have to use that style if you'd rather not. > You could continue to use gsi_end_p, or just *it, since we know that *it > is NULL at the end of the sequence. > >>> I can't find anything terribly wrong with the iterator but let me >>> at least pick on some nits ;) >>> >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, >>>>> operator--, operator*, operator==, and operator!=. >>>>> (class tsi_range): New. >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * constexpr.c (build_data_member_initialization): Use tsi_range. >>>>> (build_constexpr_constructor_member_initializers): Likewise. >>>>> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. >>>>> (potential_constant_expression_1): Likewise. >>>>> * coroutines.cc (await_statement_expander): Likewise. >>>>> (await_statement_walker): Likewise. >>>>> * module.cc (trees_out::core_vals): Likewise. >>>>> * pt.c (tsubst_expr): Likewise. >>>>> * semantics.c (set_cleanup_locs): Likewise. >>>>> --- >>>>> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- >>>>> gcc/cp/constexpr.c | 42 >>>>> ++++++++++++++---------------------------- >>>>> gcc/cp/coroutines.cc | 10 ++++------ >>>>> gcc/cp/module.cc | 5 ++--- >>>>> gcc/cp/pt.c | 5 ++--- >>>>> gcc/cp/semantics.c | 5 ++--- >>>>> 6 files changed, 47 insertions(+), 48 deletions(-) >>>>> >>>>> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h >>>>> index 076fff8644c..f57456bb473 100644 >>>>> --- a/gcc/tree-iterator.h >>>>> +++ b/gcc/tree-iterator.h >>>>> @@ -1,4 +1,4 @@ >>>>> -/* Iterator routines for manipulating GENERIC tree statement list. >>>>> +/* Iterator routines for manipulating GENERIC tree statement list. >>>>> -*- C++ -*- >>>>> Copyright (C) 2003-2021 Free Software Foundation, Inc. >>>>> Contributed by Andrew MacLeod <amacleod@redhat.com> >>>>> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see >>>>> struct tree_stmt_iterator { >>>>> struct tree_statement_list_node *ptr; >>>>> tree container; >>> >>> I assume the absence of ctors is intentional. If so, I suggest >>> to add a comment explaing why. Otherwise, I would provide one >>> (or as many as needed). >>> >>>>> + >>>>> + bool operator== (tree_stmt_iterator b) const >>>>> + { return b.ptr == ptr && b.container == container; } >>>>> + bool operator!= (tree_stmt_iterator b) const { return !(*this == >>>>> b); } >>>>> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return >>>>> *this; } >>>>> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return >>>>> *this; } >>> >>> I would suggest to add postincrement and postdecrement. >>> >>>>> + tree &operator* () { return ptr->stmt; } >>> >>> Given the pervasive lack of const-safety in GCC and the by-value >>> semantics of the iterator this probably isn't worth it but maybe >>> add a const overload. operator-> would probably never be used. >>> >>>>> }; >>>>> static inline tree_stmt_iterator >>>>> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) >>>>> static inline void >>>>> tsi_next (tree_stmt_iterator *i) >>>>> { >>>>> - i->ptr = i->ptr->next; >>>>> + ++(*i); >>>>> } >>>>> static inline void >>>>> tsi_prev (tree_stmt_iterator *i) >>>>> { >>>>> - i->ptr = i->ptr->prev; >>>>> + --(*i); >>>>> } >>>>> static inline tree * >>>>> tsi_stmt_ptr (tree_stmt_iterator i) >>>>> { >>>>> - return &i.ptr->stmt; >>>>> + return &(*i); >>>>> } >>>>> static inline tree >>>>> tsi_stmt (tree_stmt_iterator i) >>>>> { >>>>> - return i.ptr->stmt; >>>>> + return *i; >>>>> } >>>>> +/* Make tree_stmt_iterator work as a C++ range, e.g. >>>>> + for (tree stmt : tsi_range (stmt_list)) { ... } */ >>>>> +class tsi_range >>>>> +{ >>>>> + tree t; >>>>> + public: >>>>> + tsi_range (tree t): t(t) { } >>>>> + tree_stmt_iterator begin() { return tsi_start (t); } >>>>> + tree_stmt_iterator end() { return { nullptr, t }; } >>> >>> Those member functions could be made const. >>> >>> Martin >>> >>>>> +}; >>>>> + >>>>> enum tsi_iterator_update >>>>> { >>>>> TSI_NEW_STMT, /* Only valid when single statement is >>>>> added, >>>>> move >>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>>> index 9481a5bfd3c..260b0122f59 100644 >>>>> --- a/gcc/cp/constexpr.c >>>>> +++ b/gcc/cp/constexpr.c >>>>> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, >>>>> vec<constructor_elt, va_gc> **vec) >>>>> return false; >>>>> if (TREE_CODE (t) == STATEMENT_LIST) >>>>> { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>>> - { >>>>> - if (! build_data_member_initialization (tsi_stmt (i), vec)) >>>>> - return false; >>>>> - } >>>>> + for (tree stmt : tsi_range (t)) >>>>> + if (! build_data_member_initialization (stmt, vec)) >>>>> + return false; >>>>> return true; >>>>> } >>>>> if (TREE_CODE (t) == CLEANUP_STMT) >>>>> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers >>>>> (tree type, tree body) >>>>> break; >>>>> case STATEMENT_LIST: >>>>> - for (tree_stmt_iterator i = tsi_start (body); >>>>> - !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree stmt : tsi_range (body)) >>>>> { >>>>> - body = tsi_stmt (i); >>>>> + body = stmt; >>>>> if (TREE_CODE (body) == BIND_EXPR) >>>>> break; >>>>> } >>>>> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers >>>>> (tree type, tree body) >>>>> } >>>>> else if (TREE_CODE (body) == STATEMENT_LIST) >>>>> { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree stmt : tsi_range (body)) >>>>> { >>>>> - ok = build_data_member_initialization (tsi_stmt (i), &vec); >>>>> + ok = build_data_member_initialization (stmt, &vec); >>>>> if (!ok) >>>>> break; >>>>> } >>>>> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) >>>>> { >>>>> case STATEMENT_LIST: >>>>> { >>>>> - tree_stmt_iterator i; >>>>> tree expr = NULL_TREE; >>>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree stmt : tsi_range (body)) >>>>> { >>>>> - tree s = constexpr_fn_retval (tsi_stmt (i)); >>>>> + tree s = constexpr_fn_retval (stmt); >>>>> if (s == error_mark_node) >>>>> return error_mark_node; >>>>> else if (s == NULL_TREE) >>>>> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx >>>>> *ctx, tree t, >>>>> bool *non_constant_p, bool *overflow_p, >>>>> tree *jump_target) >>>>> { >>>>> - tree_stmt_iterator i; >>>>> tree local_target; >>>>> /* In a statement-expression we want to return the last value. >>>>> For empty statement expression return void_node. */ >>>>> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx >>>>> *ctx, tree t, >>>>> local_target = NULL_TREE; >>>>> jump_target = &local_target; >>>>> } >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree stmt : tsi_range (t)) >>>>> { >>>>> - tree stmt = tsi_stmt (i); >>>>> /* We've found a continue, so skip everything until we reach >>>>> the label its jumping to. */ >>>>> if (continues (jump_target)) >>>>> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool >>>>> want_rval, bool strict, bool now, >>>>> } >>>>> case STATEMENT_LIST: >>>>> - { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>>> - { >>>>> - if (!RECUR (tsi_stmt (i), any)) >>>>> - return false; >>>>> - } >>>>> - return true; >>>>> - } >>>>> - break; >>>>> + for (tree stmt : tsi_range (t)) >>>>> + if (!RECUR (stmt, any)) >>>>> + return false; >>>>> + return true; >>>>> case MODIFY_EXPR: >>>>> if (cxx_dialect < cxx14) >>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc >>>>> index dbd703a67cc..9b498f9d0b4 100644 >>>>> --- a/gcc/cp/coroutines.cc >>>>> +++ b/gcc/cp/coroutines.cc >>>>> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int >>>>> *do_subtree, void *d) >>>>> return NULL_TREE; /* Just process the sub-trees. */ >>>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>>>> { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree &s : tsi_range (*stmt)) >>>>> { >>>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, >>>>> + res = cp_walk_tree (&s, await_statement_expander, >>>>> d, NULL); >>>>> if (res) >>>>> return res; >>>>> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int >>>>> *do_subtree, void *d) >>>>> } >>>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) >>>>> { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) >>>>> + for (tree &s : tsi_range (*stmt)) >>>>> { >>>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, >>>>> + res = cp_walk_tree (&s, await_statement_walker, >>>>> d, NULL); >>>>> if (res) >>>>> return res; >>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>>> index 02c19f55548..f0fb0144706 100644 >>>>> --- a/gcc/cp/module.cc >>>>> +++ b/gcc/cp/module.cc >>>>> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) >>>>> break; >>>>> case STATEMENT_LIST: >>>>> - for (tree_stmt_iterator iter = tsi_start (t); >>>>> - !tsi_end_p (iter); tsi_next (&iter)) >>>>> - if (tree stmt = tsi_stmt (iter)) >>>>> + for (tree stmt : tsi_range (t)) >>>>> + if (stmt) >>>>> WT (stmt); >>>>> WT (NULL_TREE); >>>>> break; >>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c >>>>> index 116bdd2e42a..ad140cfd586 100644 >>>>> --- a/gcc/cp/pt.c >>>>> +++ b/gcc/cp/pt.c >>>>> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t >>>>> complain, tree in_decl, >>>>> { >>>>> case STATEMENT_LIST: >>>>> { >>>>> - tree_stmt_iterator i; >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) >>>>> - RECUR (tsi_stmt (i)); >>>>> + for (tree stmt : tsi_range (t)) >>>>> + RECUR (stmt); >>>>> break; >>>>> } >>>>> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c >>>>> index 6224f49f189..2912efad9be 100644 >>>>> --- a/gcc/cp/semantics.c >>>>> +++ b/gcc/cp/semantics.c >>>>> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) >>>>> set_cleanup_locs (CLEANUP_BODY (stmts), loc); >>>>> } >>>>> else if (TREE_CODE (stmts) == STATEMENT_LIST) >>>>> - for (tree_stmt_iterator i = tsi_start (stmts); >>>>> - !tsi_end_p (i); tsi_next (&i)) >>>>> - set_cleanup_locs (tsi_stmt (i), loc); >>>>> + for (tree stmt : tsi_range (stmts)) >>>>> + set_cleanup_locs (stmt, loc); >>>>> } >>>>> /* Finish a scope. */ >>>>> >>>>> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c >>>>> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator 2021-05-26 13:56 ` Jason Merrill @ 2021-05-28 6:53 ` Richard Biener 0 siblings, 0 replies; 8+ messages in thread From: Richard Biener @ 2021-05-28 6:53 UTC (permalink / raw) To: Jason Merrill; +Cc: Martin Sebor, GCC Patches On Wed, May 26, 2021 at 3:56 PM Jason Merrill <jason@redhat.com> wrote: > > Ping. The non-C++ parts are OK. Richard. > On 5/17/21 1:58 PM, Jason Merrill wrote: > > On 5/17/21 3:56 AM, Richard Biener wrote: > >> On Fri, May 14, 2021 at 2:23 AM Martin Sebor via Gcc-patches > >> <gcc-patches@gcc.gnu.org> wrote: > >>> > >>> On 5/13/21 1:26 PM, Jason Merrill via Gcc-patches wrote: > >>>> Ping. > >>>> > >>>> On 5/1/21 12:29 PM, Jason Merrill wrote: > >>>>> Like my recent patch to add ovl_range and lkp_range in the C++ > >>>>> front end, > >>>>> this patch adds the tsi_range adaptor for using C++11 range-based > >>>>> 'for' with > >>>>> a STATEMENT_LIST, e.g. > >>>>> > >>>>> for (tree stmt : tsi_range (stmt_list)) { ... } > >>>>> > >>>>> This also involves adding some operators to tree_stmt_iterator that > >>>>> are > >>>>> needed for range-for iterators, and should also be useful in code that > >>>>> uses > >>>>> the iterators directly. > >>>>> > >>>>> The patch updates the suitable loops in the C++ front end, but does > >>>>> not > >>>>> touch any loops elsewhere in the compiler. > >>> > >>> I like the modernization of the loops. > >> > >> The only worry I have (and why I stopped looking at range-for) is that > >> this adds another style of looping over stmts without opening the > >> possibility to remove another or even unify all of them. That's because > >> range-for isn't powerful enough w/o jumping through hoops and/or > >> we cannot use what appearantly ranges<> was intended for (fix > >> this limitation). > > > > The range-for enabled by my patch simplifies the common case of simple > > iteration over elements; that seems worth doing to me even if it doesn't > > replace all loops. Just as FOR_EACH_VEC_ELT isn't suitable for all > > loops over a vector. > > > >> That said, if some C++ literate could see if for example > >> what gimple-iterator.h provides can be completely modernized > >> then that would be great of course. > >> > >> There's stuff like reverse iteration > > > > This is typically done with the reverse_iterator<> adaptor, which we > > could get from <iterator> or duplicate. I didn't see enough reverse > > iterations to make it seem worth the bother. > > > >> iteration skipping debug stmts, > > > > There you can move the condition into the loop: > > > > if (gimple_code (stmt) == GIMPLE_DEBUG) > > continue; > > > >> compares of iterators like gsi_one_before_end_p, etc. > > > > Certainly anything where you want to mess with the iterators directly > > doesn't really translate to range-for. > > > >> Given my failed tries (but I'm a C++ illiterate) my TODO list now > >> only contains turning the iterators into STL style ones, thus > >> gsi_stmt (it) -> *it, gsi_next (&it) -> ++it, etc. - but even > >> it != end_p looks a bit awkward there. > > > > Well, it < end_val is pretty typical for loops involving integer > > iterators. But you don't have to use that style if you'd rather not. > > You could continue to use gsi_end_p, or just *it, since we know that *it > > is NULL at the end of the sequence. > > > >>> I can't find anything terribly wrong with the iterator but let me > >>> at least pick on some nits ;) > >>> > >>>>> > >>>>> gcc/ChangeLog: > >>>>> > >>>>> * tree-iterator.h (struct tree_stmt_iterator): Add operator++, > >>>>> operator--, operator*, operator==, and operator!=. > >>>>> (class tsi_range): New. > >>>>> > >>>>> gcc/cp/ChangeLog: > >>>>> > >>>>> * constexpr.c (build_data_member_initialization): Use tsi_range. > >>>>> (build_constexpr_constructor_member_initializers): Likewise. > >>>>> (constexpr_fn_retval, cxx_eval_statement_list): Likewise. > >>>>> (potential_constant_expression_1): Likewise. > >>>>> * coroutines.cc (await_statement_expander): Likewise. > >>>>> (await_statement_walker): Likewise. > >>>>> * module.cc (trees_out::core_vals): Likewise. > >>>>> * pt.c (tsubst_expr): Likewise. > >>>>> * semantics.c (set_cleanup_locs): Likewise. > >>>>> --- > >>>>> gcc/tree-iterator.h | 28 +++++++++++++++++++++++----- > >>>>> gcc/cp/constexpr.c | 42 > >>>>> ++++++++++++++---------------------------- > >>>>> gcc/cp/coroutines.cc | 10 ++++------ > >>>>> gcc/cp/module.cc | 5 ++--- > >>>>> gcc/cp/pt.c | 5 ++--- > >>>>> gcc/cp/semantics.c | 5 ++--- > >>>>> 6 files changed, 47 insertions(+), 48 deletions(-) > >>>>> > >>>>> diff --git a/gcc/tree-iterator.h b/gcc/tree-iterator.h > >>>>> index 076fff8644c..f57456bb473 100644 > >>>>> --- a/gcc/tree-iterator.h > >>>>> +++ b/gcc/tree-iterator.h > >>>>> @@ -1,4 +1,4 @@ > >>>>> -/* Iterator routines for manipulating GENERIC tree statement list. > >>>>> +/* Iterator routines for manipulating GENERIC tree statement list. > >>>>> -*- C++ -*- > >>>>> Copyright (C) 2003-2021 Free Software Foundation, Inc. > >>>>> Contributed by Andrew MacLeod <amacleod@redhat.com> > >>>>> @@ -32,6 +32,13 @@ along with GCC; see the file COPYING3. If not see > >>>>> struct tree_stmt_iterator { > >>>>> struct tree_statement_list_node *ptr; > >>>>> tree container; > >>> > >>> I assume the absence of ctors is intentional. If so, I suggest > >>> to add a comment explaing why. Otherwise, I would provide one > >>> (or as many as needed). > >>> > >>>>> + > >>>>> + bool operator== (tree_stmt_iterator b) const > >>>>> + { return b.ptr == ptr && b.container == container; } > >>>>> + bool operator!= (tree_stmt_iterator b) const { return !(*this == > >>>>> b); } > >>>>> + tree_stmt_iterator &operator++ () { ptr = ptr->next; return > >>>>> *this; } > >>>>> + tree_stmt_iterator &operator-- () { ptr = ptr->prev; return > >>>>> *this; } > >>> > >>> I would suggest to add postincrement and postdecrement. > >>> > >>>>> + tree &operator* () { return ptr->stmt; } > >>> > >>> Given the pervasive lack of const-safety in GCC and the by-value > >>> semantics of the iterator this probably isn't worth it but maybe > >>> add a const overload. operator-> would probably never be used. > >>> > >>>>> }; > >>>>> static inline tree_stmt_iterator > >>>>> @@ -71,27 +78,38 @@ tsi_one_before_end_p (tree_stmt_iterator i) > >>>>> static inline void > >>>>> tsi_next (tree_stmt_iterator *i) > >>>>> { > >>>>> - i->ptr = i->ptr->next; > >>>>> + ++(*i); > >>>>> } > >>>>> static inline void > >>>>> tsi_prev (tree_stmt_iterator *i) > >>>>> { > >>>>> - i->ptr = i->ptr->prev; > >>>>> + --(*i); > >>>>> } > >>>>> static inline tree * > >>>>> tsi_stmt_ptr (tree_stmt_iterator i) > >>>>> { > >>>>> - return &i.ptr->stmt; > >>>>> + return &(*i); > >>>>> } > >>>>> static inline tree > >>>>> tsi_stmt (tree_stmt_iterator i) > >>>>> { > >>>>> - return i.ptr->stmt; > >>>>> + return *i; > >>>>> } > >>>>> +/* Make tree_stmt_iterator work as a C++ range, e.g. > >>>>> + for (tree stmt : tsi_range (stmt_list)) { ... } */ > >>>>> +class tsi_range > >>>>> +{ > >>>>> + tree t; > >>>>> + public: > >>>>> + tsi_range (tree t): t(t) { } > >>>>> + tree_stmt_iterator begin() { return tsi_start (t); } > >>>>> + tree_stmt_iterator end() { return { nullptr, t }; } > >>> > >>> Those member functions could be made const. > >>> > >>> Martin > >>> > >>>>> +}; > >>>>> + > >>>>> enum tsi_iterator_update > >>>>> { > >>>>> TSI_NEW_STMT, /* Only valid when single statement is > >>>>> added, > >>>>> move > >>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > >>>>> index 9481a5bfd3c..260b0122f59 100644 > >>>>> --- a/gcc/cp/constexpr.c > >>>>> +++ b/gcc/cp/constexpr.c > >>>>> @@ -330,12 +330,9 @@ build_data_member_initialization (tree t, > >>>>> vec<constructor_elt, va_gc> **vec) > >>>>> return false; > >>>>> if (TREE_CODE (t) == STATEMENT_LIST) > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >>>>> - { > >>>>> - if (! build_data_member_initialization (tsi_stmt (i), vec)) > >>>>> - return false; > >>>>> - } > >>>>> + for (tree stmt : tsi_range (t)) > >>>>> + if (! build_data_member_initialization (stmt, vec)) > >>>>> + return false; > >>>>> return true; > >>>>> } > >>>>> if (TREE_CODE (t) == CLEANUP_STMT) > >>>>> @@ -577,10 +574,9 @@ build_constexpr_constructor_member_initializers > >>>>> (tree type, tree body) > >>>>> break; > >>>>> case STATEMENT_LIST: > >>>>> - for (tree_stmt_iterator i = tsi_start (body); > >>>>> - !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree stmt : tsi_range (body)) > >>>>> { > >>>>> - body = tsi_stmt (i); > >>>>> + body = stmt; > >>>>> if (TREE_CODE (body) == BIND_EXPR) > >>>>> break; > >>>>> } > >>>>> @@ -617,10 +613,9 @@ build_constexpr_constructor_member_initializers > >>>>> (tree type, tree body) > >>>>> } > >>>>> else if (TREE_CODE (body) == STATEMENT_LIST) > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree stmt : tsi_range (body)) > >>>>> { > >>>>> - ok = build_data_member_initialization (tsi_stmt (i), &vec); > >>>>> + ok = build_data_member_initialization (stmt, &vec); > >>>>> if (!ok) > >>>>> break; > >>>>> } > >>>>> @@ -675,11 +670,10 @@ constexpr_fn_retval (tree body) > >>>>> { > >>>>> case STATEMENT_LIST: > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> tree expr = NULL_TREE; > >>>>> - for (i = tsi_start (body); !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree stmt : tsi_range (body)) > >>>>> { > >>>>> - tree s = constexpr_fn_retval (tsi_stmt (i)); > >>>>> + tree s = constexpr_fn_retval (stmt); > >>>>> if (s == error_mark_node) > >>>>> return error_mark_node; > >>>>> else if (s == NULL_TREE) > >>>>> @@ -5772,7 +5766,6 @@ cxx_eval_statement_list (const constexpr_ctx > >>>>> *ctx, tree t, > >>>>> bool *non_constant_p, bool *overflow_p, > >>>>> tree *jump_target) > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> tree local_target; > >>>>> /* In a statement-expression we want to return the last value. > >>>>> For empty statement expression return void_node. */ > >>>>> @@ -5782,9 +5775,8 @@ cxx_eval_statement_list (const constexpr_ctx > >>>>> *ctx, tree t, > >>>>> local_target = NULL_TREE; > >>>>> jump_target = &local_target; > >>>>> } > >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree stmt : tsi_range (t)) > >>>>> { > >>>>> - tree stmt = tsi_stmt (i); > >>>>> /* We've found a continue, so skip everything until we reach > >>>>> the label its jumping to. */ > >>>>> if (continues (jump_target)) > >>>>> @@ -8283,16 +8275,10 @@ potential_constant_expression_1 (tree t, bool > >>>>> want_rval, bool strict, bool now, > >>>>> } > >>>>> case STATEMENT_LIST: > >>>>> - { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >>>>> - { > >>>>> - if (!RECUR (tsi_stmt (i), any)) > >>>>> - return false; > >>>>> - } > >>>>> - return true; > >>>>> - } > >>>>> - break; > >>>>> + for (tree stmt : tsi_range (t)) > >>>>> + if (!RECUR (stmt, any)) > >>>>> + return false; > >>>>> + return true; > >>>>> case MODIFY_EXPR: > >>>>> if (cxx_dialect < cxx14) > >>>>> diff --git a/gcc/cp/coroutines.cc b/gcc/cp/coroutines.cc > >>>>> index dbd703a67cc..9b498f9d0b4 100644 > >>>>> --- a/gcc/cp/coroutines.cc > >>>>> +++ b/gcc/cp/coroutines.cc > >>>>> @@ -1771,10 +1771,9 @@ await_statement_expander (tree *stmt, int > >>>>> *do_subtree, void *d) > >>>>> return NULL_TREE; /* Just process the sub-trees. */ > >>>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree &s : tsi_range (*stmt)) > >>>>> { > >>>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_expander, > >>>>> + res = cp_walk_tree (&s, await_statement_expander, > >>>>> d, NULL); > >>>>> if (res) > >>>>> return res; > >>>>> @@ -3523,10 +3522,9 @@ await_statement_walker (tree *stmt, int > >>>>> *do_subtree, void *d) > >>>>> } > >>>>> else if (TREE_CODE (*stmt) == STATEMENT_LIST) > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (*stmt); !tsi_end_p (i); tsi_next (&i)) > >>>>> + for (tree &s : tsi_range (*stmt)) > >>>>> { > >>>>> - res = cp_walk_tree (tsi_stmt_ptr (i), await_statement_walker, > >>>>> + res = cp_walk_tree (&s, await_statement_walker, > >>>>> d, NULL); > >>>>> if (res) > >>>>> return res; > >>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc > >>>>> index 02c19f55548..f0fb0144706 100644 > >>>>> --- a/gcc/cp/module.cc > >>>>> +++ b/gcc/cp/module.cc > >>>>> @@ -6094,9 +6094,8 @@ trees_out::core_vals (tree t) > >>>>> break; > >>>>> case STATEMENT_LIST: > >>>>> - for (tree_stmt_iterator iter = tsi_start (t); > >>>>> - !tsi_end_p (iter); tsi_next (&iter)) > >>>>> - if (tree stmt = tsi_stmt (iter)) > >>>>> + for (tree stmt : tsi_range (t)) > >>>>> + if (stmt) > >>>>> WT (stmt); > >>>>> WT (NULL_TREE); > >>>>> break; > >>>>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c > >>>>> index 116bdd2e42a..ad140cfd586 100644 > >>>>> --- a/gcc/cp/pt.c > >>>>> +++ b/gcc/cp/pt.c > >>>>> @@ -18234,9 +18234,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t > >>>>> complain, tree in_decl, > >>>>> { > >>>>> case STATEMENT_LIST: > >>>>> { > >>>>> - tree_stmt_iterator i; > >>>>> - for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i)) > >>>>> - RECUR (tsi_stmt (i)); > >>>>> + for (tree stmt : tsi_range (t)) > >>>>> + RECUR (stmt); > >>>>> break; > >>>>> } > >>>>> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c > >>>>> index 6224f49f189..2912efad9be 100644 > >>>>> --- a/gcc/cp/semantics.c > >>>>> +++ b/gcc/cp/semantics.c > >>>>> @@ -613,9 +613,8 @@ set_cleanup_locs (tree stmts, location_t loc) > >>>>> set_cleanup_locs (CLEANUP_BODY (stmts), loc); > >>>>> } > >>>>> else if (TREE_CODE (stmts) == STATEMENT_LIST) > >>>>> - for (tree_stmt_iterator i = tsi_start (stmts); > >>>>> - !tsi_end_p (i); tsi_next (&i)) > >>>>> - set_cleanup_locs (tsi_stmt (i), loc); > >>>>> + for (tree stmt : tsi_range (stmts)) > >>>>> + set_cleanup_locs (stmt, loc); > >>>>> } > >>>>> /* Finish a scope. */ > >>>>> > >>>>> base-commit: 3c65858787dc52b65b26fa7018587c01510f442c > >>>>> prerequisite-patch-id: 7ce9dff1f7d449f4a13fb882bf7b1a962a56de95 > >>>>> > >>>> > >>> > >> > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-28 6:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-01 16:29 [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator Jason Merrill 2021-05-13 19:26 ` Jason Merrill 2021-05-13 23:21 ` Martin Sebor 2021-05-14 1:50 ` Jason Merrill 2021-05-17 7:56 ` Richard Biener 2021-05-17 17:58 ` Jason Merrill 2021-05-26 13:56 ` Jason Merrill 2021-05-28 6:53 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).