From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x533.google.com (mail-ed1-x533.google.com [IPv6:2a00:1450:4864:20::533]) by sourceware.org (Postfix) with ESMTPS id 10F5D3844007 for ; Mon, 17 May 2021 07:56:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 10F5D3844007 Received: by mail-ed1-x533.google.com with SMTP id n25so5719482edr.5 for ; Mon, 17 May 2021 00:56:19 -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=VHQTmv0Znrn8zntVbVvc3PTkJgkd0pmrF9Zw+864RHg=; b=lHFs35h050z+FuuR+V9ZCaEAjq5TEswzNKQW3ik2EENETVbS3pcme/00uFmQ/mCUCQ rFPEptiwSgcmEygKg51rCVuQT6FwKX98LznKqiVviRdqMOSYGaRAI2nmQ33clea4NgxD DwSghh//9zJ8YeCv5sWphu9tJWS8Cwa1bb4//5xaurllzeA7FQfqMiraWp3mKksYZqzS TXmHf+IWQu2sXuPyyTJ2ldyIdvijDFR2NX2PuzrCor8AYATy7FbQL5T6R9r2IyjEzSdz ECOgo43R6Zy721W1bKnYIRzf95WdmPCahP6X0jWBRaELrGhrJ0DZ9hp98NXFQs6Dtt9K 6Eeg== X-Gm-Message-State: AOAM530TEEIw6jce5wjD78f590MNJWhL+YGf/tv1QPBu1IiJwLVNfZcQ Fjc5xDqwNSbe676yfwNn9YWEz7n4/pL9ExAXANI= X-Google-Smtp-Source: ABdhPJwlb2MEi7A/OjdABxd4QSdwBAYg5TbaH7uSRgHdZKfkMbbBaueBj/stpy6qytcmXke3+9DYUXWfG3ciCRyjOjU= X-Received: by 2002:aa7:cf06:: with SMTP id a6mr7134317edy.138.1621238177997; Mon, 17 May 2021 00:56:17 -0700 (PDT) MIME-Version: 1.0 References: <20210501162901.3164931-1-jason@redhat.com> In-Reply-To: From: Richard Biener Date: Mon, 17 May 2021 09:56:07 +0200 Message-ID: Subject: Re: [PATCH RFA] tree-iterator: C++11 range-for and tree_stmt_iterator To: Martin Sebor Cc: Jason Merrill , GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.0 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: Mon, 17 May 2021 07:56:21 -0000 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). 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 > >> @@ -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 > >> > > >