| Message ID | 5772732.lOV4Wx5bFT@excalibur |
|---|---|
| State | New |
| Headers | show |
| Series | Fix hash_map::traverse overload | expand |
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 &,
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 ──────────────────────────────────────────────────────────────────────────