On 11/14/2017 02:30 PM, Jakub Jelinek wrote: > On Tue, Nov 14, 2017 at 02:24:28PM -0700, Martin Sebor wrote: >> On 11/14/2017 02:04 PM, Jakub Jelinek wrote: >>> Hi! >>> >>> strlen_to_stridx.get (rhs1) returns an address into the hash_map, and >>> strlen_to_stridx.put (lhs, *ps); (in order to be efficient) doesn't make a >>> copy of the argument just in case, first inserts the slot into it which >>> may cause reallocation, and only afterwards runs the copy ctor to assign >>> the value into the new slot. So, passing it a reference to something >>> in the hash_map is wrong. Fixed thusly, bootstrapped/regtested on >>> x86_64-linux and i686-linux, ok for trunk? >> >> This seems like an unnecessary gotcha that should be possible to >> avoid in the hash_map. The corresponding standard containers >> require it to work and so it's surprising when it doesn't in GCC. >> >> I've been looking at how this is implemented and it seems to me >> that a fix should be doable by having the hash_map check to see if >> the underlying table needs to expand and if so, create a temporary >> copy of the element before reallocating it. > > That would IMHO just slow down and enlarge the hash_map for all users, > even when most of them don't really need it. > While it is reasonable for STL containers to make sure it works, we > aren't using STL containers and can pose additional restrictions. But when we make our containers behave differently than the STL it makes it much easier for someone to make a mistake such as this one.
IMHO this kind of difference in behavior is silly and long term just makes our jobs harder. I'd vote for fixing our containers. jeff
