Message ID | 5772732.lOV4Wx5bFT@excalibur |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B67543858428 for <patchwork@sourceware.org>; Mon, 6 Dec 2021 10:47:29 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from lxmtout1.gsi.de (lxmtout1.gsi.de [140.181.3.111]) by sourceware.org (Postfix) with ESMTPS id C4D303858D28 for <gcc-patches@gcc.gnu.org>; Mon, 6 Dec 2021 10:47:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C4D303858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gsi.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gsi.de Received: from localhost (localhost [127.0.0.1]) by lxmtout1.gsi.de (Postfix) with ESMTP id C82AA2050D02 for <gcc-patches@gcc.gnu.org>; Mon, 6 Dec 2021 11:47:10 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at lxmtout1.gsi.de Received: from lxmtout1.gsi.de ([127.0.0.1]) by localhost (lxmtout1.gsi.de [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 1RirVkUJFB-C for <gcc-patches@gcc.gnu.org>; Mon, 6 Dec 2021 11:47:10 +0100 (CET) Received: from srvex3.campus.gsi.de (unknown [10.10.4.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) by lxmtout1.gsi.de (Postfix) with ESMTPS id AE6D72050D00 for <gcc-patches@gcc.gnu.org>; Mon, 6 Dec 2021 11:47:10 +0100 (CET) Received: from excalibur.localnet (140.181.3.12) by srvex3.campus.gsi.de (10.10.4.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2375.17; Mon, 6 Dec 2021 11:47:10 +0100 From: Matthias Kretz <m.kretz@gsi.de> To: <gcc-patches@gcc.gnu.org> Subject: [PATCH] Fix hash_map::traverse overload Date: Mon, 6 Dec 2021 11:47:10 +0100 Message-ID: <5772732.lOV4Wx5bFT@excalibur> Organization: GSI Helmholtzzentrum =?utf-8?q?f=C3=BCr?= Schwerionenforschung MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart4698020.31r3eYUQgx" Content-Transfer-Encoding: 7Bit X-Originating-IP: [140.181.3.12] X-ClientProxiedBy: srvex3.Campus.gsi.de (10.10.4.16) To srvex3.campus.gsi.de (10.10.4.16) X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Fix hash_map::traverse overload
|
|
Commit Message
Matthias Kretz
Dec. 6, 2021, 10:47 a.m. UTC
While reading the hash_map code I noticed this inconsistency. Bootstrapped and
regtested on x86_64. OK for trunk?
The hash_map::traverse overload taking a non-const Value pointer breaks
if the callback returns false. The other overload should behave the
same.
Signed-off-by: Matthias Kretz <m.kretz@gsi.de>
gcc/ChangeLog:
* hash-map.h (hash_map::traverse): Let both overloads behave the
same.
---
gcc/hash-map.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--
──────────────────────────────────────────────────────────────────────────
Dr. Matthias Kretz https://mattkretz.github.io
GSI Helmholtz Centre for Heavy Ion Research https://gsi.de
stdₓ::simd
──────────────────────────────────────────────────────────────────────────
Comments
On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > While reading the hash_map code I noticed this inconsistency. Bootstrapped and > regtested on x86_64. OK for trunk? I've inspected two users of said overload and they return true. Did you look at the rest? I assume that bootstrapping and testing with asserting that the callback never returns false in that overload should succeed? That said, the inconsistency is bad - but how can we be sure we're not relying on that? I mean more than just bootstrapping and regtesting ;) Thanks, Richard. > > The hash_map::traverse overload taking a non-const Value pointer breaks > if the callback returns false. The other overload should behave the > same. > > Signed-off-by: Matthias Kretz <m.kretz@gsi.de> > > gcc/ChangeLog: > > * hash-map.h (hash_map::traverse): Let both overloads behave the > same. > --- > gcc/hash-map.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > > -- > ────────────────────────────────────────────────────────────────────────── > Dr. Matthias Kretz https://mattkretz.github.io > GSI Helmholtz Centre for Heavy Ion Research https://gsi.de > stdₓ::simd > ──────────────────────────────────────────────────────────────────────────
On Tue, Dec 7, 2021 at 8:43 AM Richard Biener <richard.guenther@gmail.com> wrote: > > On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > > > While reading the hash_map code I noticed this inconsistency. Bootstrapped and > > regtested on x86_64. OK for trunk? > > I've inspected two users of said overload and they return true. Did you look > at the rest? I assume that bootstrapping and testing with asserting that > the callback never returns false in that overload should succeed? > > That said, the inconsistency is bad - but how can we be sure we're not > relying on that? I mean more than just bootstrapping and regtesting ;) Btw, can you please amend the /* Call the call back on each pair of key and value with the passed in arg. */ comment to say how the return value influences iteration? Maybe also note the traversal is unordered. Note hash-set.h has the same "problem" (a callback with a bool return but ignored result). hash-table.h "properly" handles the return. Richard. > Thanks, > Richard. > > > > > The hash_map::traverse overload taking a non-const Value pointer breaks > > if the callback returns false. The other overload should behave the > > same. > > > > Signed-off-by: Matthias Kretz <m.kretz@gsi.de> > > > > gcc/ChangeLog: > > > > * hash-map.h (hash_map::traverse): Let both overloads behave the > > same. > > --- > > gcc/hash-map.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > -- > > ────────────────────────────────────────────────────────────────────────── > > Dr. Matthias Kretz https://mattkretz.github.io > > GSI Helmholtz Centre for Heavy Ion Research https://gsi.de > > stdₓ::simd > > ──────────────────────────────────────────────────────────────────────────
On Tuesday, 7 December 2021 08:47:56 CET Richard Biener wrote: > On Tue, Dec 7, 2021 at 8:43 AM Richard Biener > <richard.guenther@gmail.com> wrote: > > > > > > > On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > > > > > > > > > > While reading the hash_map code I noticed this inconsistency. > > > Bootstrapped and regtested on x86_64. OK for trunk? > > > > > > > > I've inspected two users of said overload and they return true. Did you > > look at the rest? I assume that bootstrapping and testing with > > asserting that the callback never returns false in that overload should > > succeed?> > > > > That said, the inconsistency is bad - but how can we be sure we're not > > relying on that? I mean more than just bootstrapping and regtesting ;) I could change the call back signature to return void. That would catch all uses, which I would modify to return void instead of true. If anyone then needs another overload which can break a correct overload can be added back. > Btw, can you please amend the > > /* Call the call back on each pair of key and value with the passed in > arg. */ > > comment to say how the return value influences iteration? Maybe also > note the traversal is unordered. OK. > Note hash-set.h has the same "problem" (a callback with a bool return > but ignored result). hash-table.h "properly" handles the return. I'll take a look. Matthias. > > Richard. > > > > Thanks, > > Richard. > > > > > > > > > > > > > > > The hash_map::traverse overload taking a non-const Value pointer breaks > > > if the callback returns false. The other overload should behave the > > > same. > > > > > > > > > > > > Signed-off-by: Matthias Kretz <m.kretz@gsi.de> > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > * hash-map.h (hash_map::traverse): Let both overloads behave > > > the > > > same. > > > > > > --- > > > > > > gcc/hash-map.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > -- > > > ──────────────────────────────────────────────────────────────────────── > > > ── > > > > > > Dr. Matthias Kretz > > > https://mattkretz.github.io > > > GSI Helmholtz Centre for Heavy Ion Research > > > https://gsi.de > > > stdₓ::simd > > > > > > ──────────────────────────────────────────────────────────────────────── > > > ──
On Tue, Dec 7, 2021 at 9:38 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > On Tuesday, 7 December 2021 08:47:56 CET Richard Biener wrote: > > On Tue, Dec 7, 2021 at 8:43 AM Richard Biener > > <richard.guenther@gmail.com> wrote: > > > > > > > > > > > On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > > > > > > > > > > > > > > While reading the hash_map code I noticed this inconsistency. > > > > Bootstrapped and regtested on x86_64. OK for trunk? > > > > > > > > > > > > I've inspected two users of said overload and they return true. Did you > > > look at the rest? I assume that bootstrapping and testing with > > > asserting that the callback never returns false in that overload should > > > succeed?> > > > > > > That said, the inconsistency is bad - but how can we be sure we're not > > > relying on that? I mean more than just bootstrapping and regtesting ;) > > I could change the call back signature to return void. That would catch all > uses, which I would modify to return void instead of true. If anyone then > needs another overload which can break a correct overload can be added back. Btw, I do like consistency between the container traversal APIs ... > > Btw, can you please amend the > > > > /* Call the call back on each pair of key and value with the passed in > > arg. */ > > > > comment to say how the return value influences iteration? Maybe also > > note the traversal is unordered. > > OK. > > > Note hash-set.h has the same "problem" (a callback with a bool return > > but ignored result). hash-table.h "properly" handles the return. > > I'll take a look. > > Matthias. > > > > > Richard. > > > > > > > Thanks, > > > Richard. > > > > > > > > > > > > > > > > > > > > > The hash_map::traverse overload taking a non-const Value pointer breaks > > > > if the callback returns false. The other overload should behave the > > > > same. > > > > > > > > > > > > > > > > Signed-off-by: Matthias Kretz <m.kretz@gsi.de> > > > > > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > > > > > * hash-map.h (hash_map::traverse): Let both overloads behave > > > > the > > > > same. > > > > > > > > --- > > > > > > > > gcc/hash-map.h | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > > > -- > > > > ──────────────────────────────────────────────────────────────────────── > > > > ── > > > > > > > > Dr. Matthias Kretz > > > > https://mattkretz.github.io > > > > GSI Helmholtz Centre for Heavy Ion Research > > > > https://gsi.de > > > > stdₓ::simd > > > > > > > > ──────────────────────────────────────────────────────────────────────── > > > > ── > > > -- > ────────────────────────────────────────────────────────────────────────── > Dr. Matthias Kretz https://mattkretz.github.io > GSI Helmholtz Centre for Heavy Ion Research https://gsi.de > stdₓ::simd > ──────────────────────────────────────────────────────────────────────────
On Tuesday, 7 December 2021 08:43:56 CET Richard Biener wrote: > On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > While reading the hash_map code I noticed this inconsistency. Bootstrapped > > and regtested on x86_64. OK for trunk? > > I've inspected two users of said overload and they return true. Did you > look at the rest? I assume that bootstrapping and testing with asserting > that the callback never returns false in that overload should succeed? > That said, the inconsistency is bad - but how can we be sure we're not > relying on that? I mean more than just bootstrapping and regtesting ;) I've checked all users now; and added more documentation. OK for trunk now? ---- 8< ---- The hash_map::traverse overload taking a non-const Value pointer breaks if the callback returns false. The other overload should behave the same. Signed-off-by: Matthias Kretz <m.kretz@gsi.de> gcc/ChangeLog: * hash-map.h (hash_map::traverse): Let both overloads behave the same. * predict.c (assert_is_empty): Return true, thus not changing behavior. --- gcc/hash-map.h | 6 ++++-- gcc/predict.c | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-)
On Tue, Dec 7, 2021 at 12:10 PM Matthias Kretz <m.kretz@gsi.de> wrote: > > On Tuesday, 7 December 2021 08:43:56 CET Richard Biener wrote: > > On Mon, Dec 6, 2021 at 11:47 AM Matthias Kretz <m.kretz@gsi.de> wrote: > > > While reading the hash_map code I noticed this inconsistency. Bootstrapped > > > and regtested on x86_64. OK for trunk? > > > > I've inspected two users of said overload and they return true. Did you > > look at the rest? I assume that bootstrapping and testing with asserting > > that the callback never returns false in that overload should succeed? > > That said, the inconsistency is bad - but how can we be sure we're not > > relying on that? I mean more than just bootstrapping and regtesting ;) > > I've checked all users now; and added more documentation. OK for trunk now? OK. Thanks, Richard. > ---- 8< ---- > > The hash_map::traverse overload taking a non-const Value pointer breaks > if the callback returns false. The other overload should behave the > same. > > Signed-off-by: Matthias Kretz <m.kretz@gsi.de> > > gcc/ChangeLog: > > * hash-map.h (hash_map::traverse): Let both overloads behave the > same. > * predict.c (assert_is_empty): Return true, thus not changing > behavior. > --- > gcc/hash-map.h | 6 ++++-- > gcc/predict.c | 2 +- > 2 files changed, 5 insertions(+), 3 deletions(-) > > > -- > ────────────────────────────────────────────────────────────────────────── > Dr. Matthias Kretz https://mattkretz.github.io > GSI Helmholtz Centre for Heavy Ion Research https://gsi.de > stdₓ::simd > ──────────────────────────────────────────────────────────────────────────
diff --git a/gcc/hash-map.h b/gcc/hash-map.h index dd039f10343..6e1c7b6e071 100644 --- a/gcc/hash-map.h +++ b/gcc/hash-map.h @@ -225,7 +225,8 @@ public: { for (typename hash_table<hash_entry>::iterator iter = m_table.begin (); iter != m_table.end (); ++iter) - f ((*iter).m_key, (*iter).m_value, a); + if (!f ((*iter).m_key, (*iter).m_value, a)) + break; } template<typename Arg, bool (*f)(const typename Traits::key_type &,