Re: [PATCH][RFC] Make iterating over hash-map elide copying/destructing

[Richard Biener <rguenther at suse dot de>]

On Tue, 10 Jul 2018, Richard Biener wrote:

> On Tue, 10 Jul 2018, Trevor Saunders wrote:
> 
> > On Tue, Jul 10, 2018 at 10:43:20AM +0200, Richard Biener wrote:
> > > 
> > > The following makes the hash-map iterator dereference return a pair<Key, 
> > > Value&> rather than a copy of Value.  This matches the hash-table iterator
> > > behavior and avoids issues with
> > > 
> > >   hash_map<tree, auto_vec<..., 2> >
> > 
> > Eventually somebodies probably going to want
> > hash_map<unique_ptr<x>>, auto_vec<tree>> too, so we might as well go ahead
> > and make it pair<key &, value &>?
> > 
> > > where iterating over the hash-table will call the auto_vec destructor
> > > when dereferencing the iterator.  I note that the copy ctor of
> > > auto_vec should probably be deleted and the hash-table/map iterators
> > > should possibly support an alternate "reference" type to the stored
> > > Values so we can use vec<> for "references" and auto_vec<> for
> > > stored members.
> > 
> > I think code somewhere uses the auto_vec copy ctor to return a auto_vec,
> > this is pretty similar to the situation with unique_ptr in c++98 mode.
> > 
> > > But that's out of scope - the patch below seems to survive minimal
> > > testing at least.
> > > 
> > > I suppose we still want to somehow hide the copy ctors of auto_vec?
> > 
> > I suspec the best we can do is delete it in c++11 mode and provide a
> > auto_vec<T>(auto_vec<T> &&) move ctor instead.  Though I think for the
> > case where auto_vec has inline storage we should be able to just delete
> > the copy ctor?
> > 
> > > How does hash-map growth work here?  (I suppose it doesn't...?)
> > 
> > Yeah was going to ask, I think hash_table memcpy's the elements? in
> > which case memcpying a pointer into yourself isn't going to work.
> 
> It doesn't work.  It uses assignment but auto_vec doesn't implement
> that so auto-storage breaks.  So you say it should use
> std::move<> where that's obviously not available for us :/
> 
> > However I think if you use the auto_vec specialization for 0 internal
> > elements that should be able to work if we null out the old auto_vec or
> > avoid running dtors on the old elements.
> 
> Well, then I don't really need auto_vec, I'm more interested in the
> embedded storage than the destructor ;)
> 
> > > Any further comments?
> > 
> > other than using a reference for the key type seems good.
> 
> OK, I suppose it should be 'const Key&' then (hopefully that
> works for Key == const X / X * as intended).

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied.

Richard.

2018-07-10  Richard Biener  <rguenther@suse.de>

	* hash-map.h (hash_map::iterator::operator*): Return
	references to key and value.

diff --git a/gcc/hash-map.h b/gcc/hash-map.h
index 7861440f3b3..39848289d80 100644
--- a/gcc/hash-map.h
+++ b/gcc/hash-map.h
@@ -223,10 +223,10 @@ public:
       return *this;
     }
 
-    std::pair<Key, Value> operator* ()
+    std::pair<const Key&, Value&> operator* ()
     {
       hash_entry &e = *m_iter;
-      return std::pair<Key, Value> (e.m_key, e.m_value);
+      return std::pair<const Key&, Value&> (e.m_key, e.m_value);
     }
 
     bool