On Tue, 14 Nov 2017, Jeff Law wrote: > 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.
I'd argue that this is simply a programming error and I doubt the libstdc++ variant works by design/specification. So let's go with Jakubs patch. We can do the extra checking as followup. Jakub, your patch is ok. Thanks, Richard.
