From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com [IPv6:2a00:1450:4864:20::62a]) by sourceware.org (Postfix) with ESMTPS id 7C9383857011 for ; Fri, 28 May 2021 06:54:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7C9383857011 Received: by mail-ej1-x62a.google.com with SMTP id ss26so3769553ejb.5 for ; Thu, 27 May 2021 23:54:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=dACSSPzpjagDiGXCcRx69yuAdTsjeLLnIPgORM/xEik=; b=FiPR03RAejGX58LOAHaC+QQ1P7CLHiE77bi2xPmAE6GJqA+toiIQVDd8gVSfnBt/Ix v2zQBNOk/PWy4l5FGR+8V2aVcPV1UCnhu6clB/VwKYNgkMrbHP9g1RG2VBQvu1EIB2Gl 3NRCoOUd8KoF4xRoU2p4XsFoe/cqISoYzUPQtZu3SKXvVhzS6aMuOKWDLivTXts4Rwr+ ALqxRPiyYRX/XtcGem4w8AXuYoxT3SnI+aktIUr6SCmcqDx/9+wYuil0OmTjYvl8FvdH iOfkDcQi8ZXM7ttEDOt6lsBiVikMTX1S58AT3yBQJ7hRtgDFa+uly9z9faTLmdRySVis XDNg== X-Gm-Message-State: AOAM5301BT7FpNYmryyIBnFNSKjlnDNSopYIPoyGUCWX+E4xPJgDc414 4EklC3Q7c4m0oLhsF/c/yhY9C6ILpgvunU8JZ/k= X-Google-Smtp-Source: ABdhPJwZYDmpzsS9/MjZXudG4ZlHJGHIs4OEl/8FxBQyp4MNByP1NjjCEI9NVtm3p+qEL4m9RGRfTv9fMkomWO9yOcE= X-Received: by 2002:a17:907:b03:: with SMTP id h3mr7616939ejl.118.1622184849962; Thu, 27 May 2021 23:54:09 -0700 (PDT) MIME-Version: 1.0 References: <20210501162901.3164931-1-jason@redhat.com> <6a94ad09-a630-ff6f-da9d-9dc4402db32e@redhat.com> In-Reply-To: From: Richard Biener Date: Fri, 28 May 2021 08:53:58 +0200 Message-ID: Subject: Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator To: Jason Merrill Cc: Martin Sebor , GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 May 2021 06:54:13 -0000 On Wed, May 26, 2021 at 3:56 PM Jason Merrill 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 > >> 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 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 > >>>>> @@ -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 **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 > >>>>> > >>>> > >>> > >> > > >