On Wed, May 26, 2021 at 3:56 PM Jason Merrill <ja...@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 <amacl...@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
> >>>>>
> >>>>
> >>>
> >>
> >
>
